Add NoDiscard attribute for Uri::with*() and Url::with*() methods

[kocsismate]

Some wither tests are going to fail.. but I'd like to merge #19970 first and then rebase this one instead of fixing the warnings.

[TimWolla]

No strong opinion either way. I don't think it is necessary to have, though (due to the much clearer naming of with…()).

[TimWolla]

This message is referring to the wrong class (same below).

[kocsismate]

No strong opinion either way. I don't think it is necessary to have, though (due to the much clearer naming of with…()).

What would be the downside of adding the annotation? Only a small performance penalty? Or is it a bit larger?

[TimWolla]

What would be the downside of adding the annotation? Only a small performance penalty? Or is it a bit larger?

Small performance penalty and possibly also “alarm fatigue”. When a larger number of functions start to warn, it might lead to users just suppressing the warning, resulting in them missing the important warnings.

In case of Uri, the following might be reasonable to “validate” components:

$uri = Uri::parse('https://example.com/');;
try {
    $uri->withPath($myPath);
    $pathValid = true,
} catch (InvalidUriException) {
    $pathValid = false,
}

I'm not saying I'm against, but I'm also not in favor. If this were an RFC, I'd “Abstain”. I thus leave the decision to someone else.

[edorian]

No strong feelings from my side.

It's late in the release/testing process, and the warnings were not part of the RFC. Given this isn't how PHP works in other places, and I didn't see any desire from folks to update these either, it feels a bit inconsistent to warn just here and not for a str_contains($foo, $bar).

So if PHP wants to do that, I'd rather see that done more globally and with an RFC and a "policy/guideline" change that all functions without side effects should do that. Instead of just adding it for this extension. I think the value is limited given the great API design.

But to be clear: If a couple of folks feel strongly in favor, I'm ok with having this in 8.5.

[DanielEScherzer]

Wearing my RM hat, I object to including this in 8.5, and for master this should probably go through an RFC

I agree that it would be useful to add \NoDiscard for functions that have no side effects, but we should do that more generally rather than just for this extension

[kocsismate]

OK neither I feel very strongly about this, but I thought it was a good idea to do this now, since this is the last time the attribute could be added without any BC break. The value is maybe limited, but I surely committed the error with withers in the past that the attribute can prevent.