In Drupal commerce, we can use store IDs like fr, tr. The functionality behaves these as language and deletes them.
For example: /fr/tr (/storeid/langid) become /tr (/langid)
Or, if you have links that include a language prefix, they can be stripped and end up breaking the links. See test failures at #2418369-62: Internal URL handling (language prefixes, base://, ...).
Add a setting to toggle the behavior if language prefixes should be stripped out of URLs. Default to leaving the URL alone for new sites, but add a post_update to set the setting to strip language prefixes for existing sites (to match current behavior).
New checkbox setting:
[ ] Keep language prefix
If not checked, URL prefixes that match configured languages will be removed from links.
See screenshot of the new settings UI.
New global and format setting schema. pathologic_post_update_set_keep_language_prefix() sets the value to FALSE on existing sites to maintain legacy behavior.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !8
Comments
Comment #3
emircan erkul commentedComment #4
emircan erkul commentedComment #5
dwwThanks for the bug report, and for trying to fix it with a patch!
Needs work because the tests are now failing with this change.
I'm also not sure I consider this a major bug, since I don't know how common it is to name your stores after country codes, but I'll leave it this way for now. 😅
Thanks again,
-Derek
Comment #6
dwwBased on my findings at #2418369-61: Internal URL handling (language prefixes, base://, ...) and beyond, I'm not clear why Pathologic should ever be stripping off language codes like it does. But I clearly don't have the full picture. It looks like we're leaning towards making it configurable. However, I believe it should default to leaving the langcode alone by default, and sites should have to opt-in to having those stripped off. I suppose we'll need an update path for existing sites to opt-in for minimum disruption with the current behavior, but announce in the release notes that folks can now turn off the stripping if they want.
I'm not sure "Keep language prefix" is the most clear as the UI and config keys for this setting. 🤔 I like that it's phrased in the "positive", so you have a checkbox that you check to turn something on. If we invert it to "strip language prefix" it's much worse UI. But "keep" seems off for some reason. "Retain" or "Preserve" could work, but are longer and more complicated. 🙁
I'm going to move some of my tests from #2418369 over here, port it to the 2.0.x branch, and maybe work on a post_update to set the setting for existing sites. Assigning to myself for now.
Thanks,
-Derek
Comment #7
dwwThe MR now has tests of the new setting. Still needs an upgrade path (and maybe upgrade path tests?), but would love a review on the approach for now.
Thanks!
-Derek
Comment #8
dwwReturned from some errands and had a chance to throw a quick post_update together. Works fine in light local testing. Other reviews + tests most welcome!
Comment #9
dwwPushed a few more commits to improve things, fleshing out UI changes in the summary, adding a link to a screenshot.
Comment #10
dwwComment #11
mark_fullmerComment #12
mark_fullmerComment #13
dwwThanks for the review! Addressed all threads. Want to re-review before I merge this?
Thanks!
-Derek
Comment #15
dwwBased on "My comments are mostly for consideration, rather than change requests. In other words, I'm not calling out anything that must be changed in order for this to proceed." and the fact that I addressed all the points raised, decided to merge this so we can proceed at #2418369: Internal URL handling (language prefixes, base://, ...) and elsewhere.
Thanks!
-Derek