-
Notifications
You must be signed in to change notification settings - Fork 237
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
less strict string matching in magento-vars.php #446
Comments
Hi @joeshelton-wagento, |
If documentation coverage would be enough and we do not want to introduce BIC, I am okay with that too. @joeshelton-wagento , would the documentation update work? |
Changes (by Magento) in this file would not effect existing projects. In fact, this function was recently changed. I don't think changes here would be considered BIC. d9785ce#diff-a82aa6e9574b747dac9152a71413a3ba Regardless, I'm in favor of whatever will prevent people from putting environment-specific configuration in repo files. The current design pattern in the file is prompting people to do this. If we could enhance the comments directly in the |
@joeshelton-wagento |
I see what you mean. I'm not in favor of people using the current function as-is. There's no way to use it without environment-specific domains being committed into a repository file. This is the worse of two evils, even if you consider the change BIC. So, I'm still in favor of totally removing it. Perhaps the best change would be to not include a "live" code example at all. The main problem is that people are mistakenly believing that the function provided is necessary to use when in fact it is optional. Example string matching functions could be provided in comments. Then people would be more likely to understand that the file can take a form of their choosing. And people who were used to the old version of the file would immediately notice the change. |
And, by the way, the behavior of that function did change. It became stricter. d9785ce#diff-a82aa6e9574b747dac9152a71413a3ba Previously a substring match starting at position 0 would return true. (eg. "example" would match "examplea.com" and "exampleb.com") After the change, a full match is necessary. |
Oh. That should not have happened. It is definitely a mistake. Thanks for pointing that out. |
And I like your idea about
Seems like it solves all problems |
i changed this file to this format, because i have many subdomains ` if (isHttpHost("examplea")){ ` |
The isHttpHost() function has very strict string matching. This requires a new block of code utilizing the function to be created for each new environment.
For example:
Also, the default function does not accommodate domain name differentiators that may occur at the end of the domain name. For example:
I propose a more flexible string matching that would allow for one block of code to identify a store in all environments. For example:
This is a better fit for Magento's dynamic system of hostname creation.
Personally, I always change the function to have more flexible string matching in my own projects. But, I notice that many people on Community Engineering Slack mistakenly assume that the function can't be changed. They rely on the function provided and are frustrated by it.
This is my own preference:
The text was updated successfully, but these errors were encountered: