-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixes variables in configuration fields not being replaced with actual value… #18067
Fixes variables in configuration fields not being replaced with actual value… #18067
Conversation
Hi @hostep. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
Hi @VladimirZaets, thank you for the review. |
Hi @hostep, thanks for collaboration. After manual testing we found the next problem: Problem: Cannot upload product image. In the backend configuration go to: General > Web > Base URLs & Base URLs (Secure) |
Hi @hostep, I am closing this PR now due to inactivity. |
Sorry for the late feedback, I was away on holiday for 3 weeks. Even though I probably won't find a decent solution, so if some core dev from Magento with deep knowledge about this part of the code could weigh in some opinion, that would be very much appreciated! Thanks! |
I can not reproduce the problem you are having, I'm also not really seeing the relevance of testing uploading an image while those settings are active in scope of this PR.
Are you sure the issue you are experiencing is caused by this PR? |
…s in the backend form fields.
49ad6d6
to
a8f1729
Compare
I just force pushed my branch with conflicts fixed which were caused by PR 19880 @VladimirZaets: any chance you can retest this? |
✔️ QA passed |
Hi @hostep, thank you for your contribution! |
…ed with actual value… #18067
…s in the backend form fields.
Description
See #15972 for a full description
Watch out: I feel very unsecure about these changes, these should best be reviewed by someone with deep knowledge about how the configuration system works in Magento I think.
The behavior described below worked fine in Magento 2.2.0 but broke since 2.2.1, by: 86d46ce & 6c9a8b2
Maybe @viktym would be an appropriate reviewer here, since he seems to have deep knowledge about that part in the code.
It would also be awesome if tests could get added to prevent this bug from re-appearing in the future. I already attempted creating a unit test for this, but failed horribly. Any help would be appreciated!
Fixed Issues (if relevant)
Manual testing scenarios
{{unsecure_base_url}}media/
in "Base URL for User Media Files" and{{secure_base_url}}media/
in "Secure Base URL for User Media Files"Contribution checklist