Exception handling on invalid URI modifications is a breaking change

[jeromegamez]

I know that the Guzzle project doesn't state that it is following SemVer, and the release notes clearly say:

• Ensure each URI modification results in a valid URI according to PSR-7 discussions. Invalid modifications will throw an exception instead of returning a wrong URI or doing some magic.
  • (new Uri)->withPath('foo')->withHost('example.com') will throw an exception because the path of a URI with an authority must start with a slash "/" or be empty

but I just wanted to give a notice that this change break projects that currently use invalid modifications (as it happened with some of mine, shame on me 🙂), in case that the usual "the app doesn't work anymore" bug report is brought to project maintainers. In my case, I had this code at a few places:

$uri = new Uri('http://domain.tld')->withPath('path/to/location');

This also affects projects that require guzzlehttp/guzzle, which specifies guzzlehttp/psr7: ^1.3.1 as a dependency.

:octocat:

[dkarlovi]

Yes, Azure Storage PHP SDK also suffers from this.

[jeskew]

The AWS SDK for PHP dodged breaking customer code only because the version constraint in its composer.json was ~1.3.1, which prevented an automatic upgrade to version 1.4 for many SDK users.

I understand that the new error state was added to prevent this library from entering incorrect or obviously wrong states, but it also seems to have broken some working code. I propose that we deprecate the 1.4 tag, remove it from packagist, and retag the current state of master as 2.0. @Tobion and @mtdowling what would you think about taking that course of action?

[kyleweishaupt]

I'm going through a breaking change as well, using the Azure PHP SDK.

[sagikazarmark]

I would say removing the tag is not an option. People already using it would have breaking code as well. IF we want a 2.0, then this is what I would do:

• create an 1.x branch
• revert the change on that branch
• tag it with 1.4.1
• tag master with 2.0

It's still an open question what we should do with this in Guzzle:

• mark 1.4.0 as conflicting version
• upgrade to 2.0

Note that 6.3 release is in progress, but there is still much to review, so we need to agree on timing.

[jeskew]

@sagikazarmark Releasing a 1.4.1 and 2.0 sounds like a good plan.

I think that Guzzle should be able to use a version constraint like ^1.3.1|^2.0 in that case, as I do not believe any tests or code in Guzzle would be directly affected by the new error state added in 1.4.0.

[jeromegamez]

After having looked into the implementation, I am wondering why this exception is thrown in the first place. The UriInterface method signatures state

The path can either be empty or absolute (starting with a slash) or rootless (not starting with a slash). Implementations MUST support all three syntaxes.

(see e.g. https://github.com/php-fig/http-message/blob/master/src/UriInterface.php#L248)

The changelog mentions "according to PSR-7 discussions" - does this mean that there will be a psr/http-message 2.0 as well?

[jeromegamez]

Until 1.4.1, a quick fix for project maintainers is adding this line to the composer.json file:

"guzzlehttp/psr7": "^1.3.1, !=1.4.0",

[sagikazarmark]

Wouldn't conflict be a more proper way?

[jeromegamez]

~As far as I know, conflict is for conflicting packages, not versions 🤔~ <- this didn't make sense :)

conflict would probably work just as fine, the line above was just what first came to my mind. In the guzzle package itself, it's probably also nicer to look at, than to have a conflict section that includes a package from the same namespace 😇

[sagikazarmark]

As @jeskew pointed a few comments above, technically nothing conflicts with Guzzle on this front, and as far as I can see he is right.

[jeromegamez]

I just tested it out with one of my projects, I'm writing down the details in case someone wants to reproduce it.

First, with the latest version (which has the line above in its composer.json)

$ composer require guzzlehttp/guzzle # will install guzzlehttp/psr7 1.4.0
$ composer require kreait/firebase-php # will not downgrade to guzzlehttp/psr7 1.3.1
$ composer update # will perform the downgrade

Then, with a testing branch, which has a "conflict": {"guzzlehttp/psr7": "1.4.0"} section:

$ composer require guzzlehttp/guzzle # will install guzzlehttp/psr7 1.4.0
$ composer require kreait/firebase-php dev-composer-conflict-test # will fail and not be installed

[sagikazarmark]

What if you try with --update-with-dependencies?

$ composer require guzzlehttp/guzzle # will install guzzlehttp/psr7 1.4.0
$ composer require --update-with-dependencies kreait/firebase-php dev-composer-conflict-test # ???

[jeromegamez]

Fails as well… it's out of scope of this issue anyways, but it was good to discuss it nonetheless.

[Tobion]

See #139 that reverts the BC break and triggers deprecations instead. I thought the magic behavior was introduced in 1.3.1 only as explained in https://github.com/guzzle/psr7/pull/94#issuecomment-231339262 but existed before as well. So I underestimated the impact it could cause. Sorry.