-
-
Notifications
You must be signed in to change notification settings - Fork 436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use transliterator_transliterate
to generate "url_key"
#4315
base: main
Are you sure you want to change the base?
Conversation
- cosmetic change - added method Mage_Catalog_Model_Category_Url
…nd Mage_Catalog_Model_Url - moved duplicate code tp parent
transliterator_transliterate
to generate "url_key"transliterator_transliterate
to generate "url_key"
# Conflicts: # tests/unit/Mage/Catalog/Model/CategoryTest.php # tests/unit/Mage/Catalog/Model/ProductTest.php # tests/unit/Mage/Catalog/Model/UrlTest.php
#1631 is already merged since many months, what is the point of creating this PR when next should be released? or maybe not? this PR, if merged, will create discrepancies between the branches since IMHO do not have any reasons to exist. |
#1631 was merged to next-branch only. b/c of breaking changes (#1631 (comment)) - and it does not work. See my last comment there. |
# Conflicts: # app/code/core/Mage/Catalog/Helper/Product/Url.php
I tested it on next and it was working for me. and anyway your answer doesn't answer my question. |
It formated the titles as explained, to default latin something, but localization never worked. This code does not work ... b/c there is no
imho next branch has a not working PR in, that has to be fixed/replaced. |
@sreichel Can you confirm if the feature is working in next? Why do we need to backport it to main? It is a BC so it should not be backported from my view as it is more of a feature than a bugfix considering the old way was accepted for many years. |
Its not a backport. #1631 in next branch does not work. It only replaces some chars as state in the PRs description. The code mentioned in https://github.com/OpenMage/magento-lts/pull/1631/files#diff-34c65e6986a386387a7489be1bb073b8b5e3f98e85cdf6e61804c3e309a56038R72-R78 does not work. This PR changes no method signatures and should be BC-safe. |
But doesn't it change the url slugs? Would you consider that BC? |
# Conflicts: # composer.lock # tests/unit/Mage/Catalog/Helper/Product/UrlTest.php
Slugs should only get generated when changing url-keys. |
Description (*)
@luigifab had a great idea to use
intl
-extension for generating url key with translating all special chars. For me the PR did not work, so tried myself.This is a non-BC-breaking try of it. It does not change method-signaturer and should be BC-safe. (I did not add all features of #1631. That could be done in another PR.)
To not reinvent the wheel for locale-mapping if have added
symfony/string
that exactly does what we need.See: https://symfony.com/doc/current/string.html#slugger
Related Pull Requests
Manual testing scenarios (*)
Test product/category before and after ...
Before url-key
After url-key
en_US
de_DE
Questions or comments
Locale config in xml should be changed to not have language-specif config in core. Any ideas?Magento already had an unsused config for it ...
Mage_Catalog_Helper_Product_Url::__construct
looks forMage::getConfig()->getNode('default/url/convert');
XML.