Skip to content
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

Merged
merged 4 commits into from Jun 6, 2020
Merged

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jun 6, 2020

As an alternative to #2663

@gmponos
Copy link
Member

gmponos commented Jun 6, 2020

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

@GrahamCampbell
Copy link
Member

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.

@Nyholm
Copy link
Member Author

Nyholm commented Jun 6, 2020

the 6.x branch is supposed to "just work"

I agree.


Comparing this PR with #2663:

  1. Both will "just work" in 99% of cases.
  2. This PR will "not work" if someone incorrectly "replaced symfony idn polyfill"
  3. [6.5] Removed reliance on internal polyfill code #2663 will "not work" when someone replaced incorrectly "symfony mbstring polyfill"

Both 2 and 3 are super rare. But this PR is simpler than #2663.

Is there anything else that differs the PRs?

@GrahamCampbell
Copy link
Member

Both will "just work" in 99% of cases.

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.

@GrahamCampbell
Copy link
Member

#2663 will "not work" when someone replaced incorrectly "symfony mbstring polyfill"

No, it would still work fine.

@GrahamCampbell
Copy link
Member

Also, this PR uses the mbstring polyfill too...

@gmponos
Copy link
Member

gmponos commented Jun 6, 2020

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 sf.. I was in favor with the solution implemented here.. but I can't say that my arguments are STRONG enough... so I will stick with what I had first in mind. :)

@Nyholm
Copy link
Member Author

Nyholm commented Jun 6, 2020

Well, no. This PR will fail for a lot of people if symfony do change the class.

So in an application that has replaced symfony/polyfill-intl-idn with an old version of intl AND Symfony changed that class and method, then yes, that application will get an exception when running their tests.

Since we are not locking ourselves in with this solution, I would like to merge it and focus on 7.

@GrahamCampbell
Copy link
Member

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.

@GrahamCampbell
Copy link
Member

And it fixes the problem with bad replacement.

Copy link
Member

@gmponos gmponos left a 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..

@Nyholm
Copy link
Member Author

Nyholm commented Jun 6, 2020

No possibility that anything could break in the future.

That is not true. We got more code to maintain ourselves. See symfony/polyfill#159

@Nyholm Nyholm merged commit 23730ab into guzzle:6.5 Jun 6, 2020
@Nyholm Nyholm deleted the intl-65 branch June 6, 2020 12:11
@Nyholm
Copy link
Member Author

Nyholm commented Jun 6, 2020

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants