[6.5] Removed reliance on internal polyfill code #2663
Conversation
GrahamCampbell
changed the title
|
Copying in any amount of other peoples code like this always feels against the spirit of using composer and library management... |
@reedy I don't think you understand the issue here?
|
|
All we are doing is copying it again, and removing the issue that Symfony's version is marked as internal so as no BC promise. Moreover, their internal marker says Guzzle is not allowed to call the code as it is in their package. |
Oh come on. I in no way suggested you were violating licenses or the spirit of open source. |
Sure. However, it looks like they forked it from a library that seems to have mostly been abandoned; not touched for 4 years, and symfony did this two years after the last commit to it. When something isn't maintained, I understand it completely. That isn't the only reason to fork though, indeed |
|
@reedy What do you suggest we do then? I already asked Symfony if they wanted to maintain a package with that class in, or make this class non-internal. They declined to do either, so either we copy the code, or we leave the polyfill version pinned. You said you didn't want the version pinned, so what else can we do... |
|
The class won't disappear in the Symfony polyfill. If you want to guard against this theoretical possibility, you could add tests to spot it early. The real-world risk is someone using To guard against this risk, you can copy/paste the code as you do. But this also means it'll be yours to maintain. E.g; the day we'll fix symfony/polyfill#159 (if we do), you'll have to sync. For the record, my recommendation is still the same: 1.unpin 2.throw a LogicException when an outdated intl is found and the polyfill is not found (which means it's been "replace"d.) @GrahamCampbell looking at the way you word your messages, it seems like you feel pretty strong about how things should be. Since I don't maintain this package (and don't want to :) ), I'm going to opt-out from the topic after this very message. Thanks for caring. |
|
I don't think anyone wants it pinned. Cause if you have another library that uses it (or even need a newer version yourself), and a newer release comes out... Conflicts ahoy! Maybe as part of your justification for doing this, include links to the conversation with "upstream", or tasks here? Rather than just doing something without any apparent justification as is currently occuring. People shouldn't have to dig through loads of commits to find out why you're doing it, especially as it seems odd you add a new library in one patch release, then plan to remove it in the next. Never mind you're pointing out it's internal, but you've released code in guzzle that uses that internal code anyway. Then are looking to remove it. It was good enough for the last release, but not now? You have to question whether that release needed making etc. I didn't say "you should absolutely not do this", but from this task, can a random third party (ie me) work out why you're doing it? Probably not. Sure, it may be crappy to duplicate and modify someone elses code (and taking on a maintenance burden and an amount of technical debt for it yourself), but if that's the only real way forward as is, then so be it. WHY you are doing something is as much use as WHAT you are doing. |
We already discussed this issue in great detail on Slack for a number of days (most of the discussion is not available on GitHub I'm afraid). This PR was just a follow up to that discussion. It is still not decided which is the final approach. That is, the other option we have is to disable idn by default and require the extension to enable it. |
Great, a walled garden. This doesn't help someone that doesn't have access or isn't privy to those discussions (irrelevant of whether I can get access or not, or if I even want it). Nor does the task say this is just a POC or similar. |
|
Actually, the Slack is a public channel. Anyone can read it. There should be a link somewhere in the docs. |
Fair enough. But how would I know to look there? Does your issue say "Per discussion in slack"? No. Depending on your POV, Slack is still a walled garden as it's not FOSS software etc. While I'm not so strongly opinioned on things like this, other people are. |
|
Depending on what you mean by docs, it's certainly not mentioned in this repo https://github.com/guzzle/guzzle/search?q=slack&unscoped_q=slack |
|
Sadly, I haven't followed the discussions either, so I don't know the details, but I do know this intl issue is with us for quite some time now. I'd focus on finding an acceptable solution for the short term, even if it's not perfect. Maintaining an internal copy of some code does not feel like a huge issue to me, given it doesn't change frequently. The polyfill is an internal detail in this case IMHO, not something that we want users to control. That being said, we can't protect everyone, but we can fail early in an environment without the appropriate dependencies. I doubt that a class will disappear in a minor version, so even if it's marked as internal. And we can always duplicate, if something happens. Ultimately, I can work with both solutions, both have pros and cons.
That's fair, it's kind of an unofficial channel on the PHP HTTP Slack, but I think it helped a lot in the past, so definitely should be added to the repo: #2674 |
|
My PoV is that I agree with Nicolas here... and we should add tests and the Get moving with version 7 and on v7
|
This reverts commit a6716ab.
|
@Nyholm and @sagikazarmark I would be willing to accept this if we accept this #2675 Since this code will only live from 6.5.5 to 7.0.0 which seems like a short term.. and we can always revert back to the polyfill if needed. |
Same as GitHub... |
Indeed. While you need an account to contribute on github, you need an account to even read anything on slack... So it's a different kind of walled garden. |
|
Fair enough. :) |
|
If this gets merged let's update the version constant here.. to be safe... |
|
Closed by #2678 |
Pinning the version is going to be a pain. I think the best course of action is just to copy over the code we need, so we control it. // @nicolas-grekas, @stof, @gmponos, @Nyholm