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
Unpin version for symfony/polyfill-intl-idn #2678
Conversation
I can approve if you wish.. but I don't have strong arguments for one approach vs the other... if you do.. or if this is what your guts tell you is right.. let's proceed |
I'd be against this PR since the 6.x branch is supposed to "just work", regardless of what extensions are installed or what version of the polyfill is loaded. |
I agree. Comparing this PR with #2663:
Both 2 and 3 are super rare. But this PR is simpler than #2663. Is there anything else that differs the PRs? |
Well, no. This PR will fail for a lot of people if symfony do change the class. The other PR will always work 100% of the time for everyone. |
No, it would still work fine. |
Also, this PR uses the mbstring polyfill too... |
We can always go back from one or the other solution.. it has also it's downsides not including the polyfill as we will need to follow the fixes of |
So in an application that has replaced Since we are not locking ourselves in with this solution, I would like to merge it and focus on 7. |
I'd like to merge the other PR and focus on 7. The other PR genuinely makes this done and duster forever. No possibility that anything could break in the future. |
And it fixes the problem with bad replacement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this gets merged let's update the version constant here on this PR.. and also have a changelog..
That is not true. We got more code to maintain ourselves. See symfony/polyfill#159 |
Thank you |
As an alternative to #2663