Fix the #31 slug delegation: it was a silent no-op without transliterate: true - #32
Open
mdon wants to merge 1 commit into
Open
Fix the #31 slug delegation: it was a silent no-op without transliterate: true#32mdon wants to merge 1 commit into
mdon wants to merge 1 commit into
Conversation
Caught by the upstream review of phoenix_kit_dashboards#5, which found the same defect there and had to repair it after merge. PhoenixKit.Utils.Slug.slugify/2 defaults :transliterate to FALSE on every core released so far. So `Slug.slugify(text)` strips every non-ASCII character — exactly what the local pipeline this change replaced did. The delegation was correct and the behaviour was unchanged. My verification missed it because I tested against local core, where I had made transliteration the default. Against the core these modules actually resolve, it was inert. That is the difference between "the code is right" and "the shipped artifact is right", and only the second one matters. Forward-compatible: once core ships the locale-aware Slug, :transliterate is accepted and ignored, so this stays correct. The pinning tests are rewritten to assert only what holds at EVERY core version. Asserting output was the second half of the same mistake: each module resolves a different core (posts' lockfile pins 1.7.189, which predates the :transliterate option entirely), so a literal expected slug merges red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #31: the slug delegation that merged there was a silent no-op.
PhoenixKit.Utils.Slug.slugify/2defaults:transliterateto false on every corereleased so far, so the arity-1 call I added strips every non-ASCII character — exactly
what the local pipeline it replaced did. A Cyrillic template name still slugged to
"".This is the same defect the reviewer caught in BeamLabEU/phoenix_kit_dashboards#5
(
ec8d080, "PR #5's slug change was a no-op"). It landed in this repo too and was notcaught here, because #31 had no pinning test.
Why my verification missed it
I tested against a local core where I had already made transliteration the default.
Against the core this module actually resolves, the change was inert. The code was right;
the shipped artifact was not, and only the second one matters.
Forward compatibility
Once core ships the locale-aware
Slug(BeamLabEU/phoenix_kit#693, merged — pending arelease),
:transliterateis accepted and ignored, so this stays correct. No secondchange needed.
Remaining limitation, stated plainly
This makes Cyrillic work on a core new enough to have
:transliterate. German andGreek still degrade until this module's
phoenix_kitfloor moves to the releasecarrying
locale_slug— the older transliterate maps Cyrillic only. That is a versionfloor decision, not a code one, so it is left to whoever cuts it.