Complaining about these for PHP 8.0+ (not 8.1/8.3+) makes sense to me.

PHPCompatibility should be about practical differences between PHP versions, not conceptual changes.

If PHP 8.0 behaves the same as PHP 7.4 then PHPCompatibility should not throw a notice, as it has no impact, regardless of what it may say in the release notes or RFC.

It should always reflect the reality on the ground, otherwise - for example - someone who needs to support PHP 8.0 but not 8.1 will be wasting their time trying to fix things they don't need to fix.

fwiw I'm aware that you're pretty much running this as a one-woman show at the moment, so if it is a lot of extra work to handle this properly, an acceptable compromise might be to ship as per your suggestion and log a ticket to do the additional work once 10.0 is released, as I'd rather we had a 'pretty good' 10.0 soon than a 'perfect' 10.0 later.

If PHP 8.0 behaves the same as PHP 7.4 then PHPCompatibility should not throw a notice, as it has no impact, regardless of what it may say in the release notes or RFC.

@MarkMaldaba With or without deprecation notice, PHP still behaves the same, it's only when certain features are removed that the behaviour of PHP changes.

Do you mean to say that you believe PHPCompatibility should not detect deprecations ?

Issuing a deprecation notice where previously a no notice was issued is a behavioural change that PHPCompatibility should be reporting on, even if the underlying behaviour hasn't changed. Same as if an error changes from E_NOTICE to E_WARNING. Even if the code behaves in the same way, these kind of changes may or may not affect you, depending on how error reporting is configured, and therefore should be reported on.

It sounds like @MarkMaldaba would prefer to report on deprecation notices not on a deprecation itself. Most usually these line up and land in the same version of PHP, so there's no differentiation required between the two. The case in this pull request seems to be the exception where the deprecation notice was missed or forgotten when the deprecation happened.

Yes, a deprecation in PHP does not include a functional change in behaviour (other than emitting a deprecation notice). I understand that PHPCompatibility issues warnings in these cases, and reserves errors for actual breaking changes. Users of PHPCompatibility are free to ignore warnings (phpcs -n ...) if they want to reduce their workload.

I know that there are frameworks (eg Magento2) which treat all notices that PHP emits (even E_USER_NOTICE) as fatal. In the case of these, PHPCompatibility reporting the deprecation even though PHP won't may be misleading. My experience is that such frameworks are slow to adopt / support newer versions of PHP anyway, so the likelihood of being inconvenienced by this particular case seems low to me. I recognise that I don't have a wide visibility into all the places / scenarios where PHP runs though.

I suggest that this pull request is merged as-is, and an issue is created here with the request to implement the required logic (which doesn't look trivial on first glance) to make the warnings only show up in the PHP versions where the corresponding notices began to be emitted. While that change (used to show in 8.0+, now only in 8.1+) may be considered a breaking change, I would be comfortable including it in a minor version bump, but probably not in a patch release.

It sounds like @MarkMaldaba would prefer to report on deprecation notices not on a deprecation itself. Most usually these line up and land in the same version of PHP, so there's no differentiation required between the two. The case in this pull request seems to be the exception where the deprecation notice was missed or forgotten when the deprecation happened.

I'm not really sure what the distinction between a deprecation and a deprecation notice is, in this context.

What I think we should report on is the time that the PHP runtime started exhibiting a change in behaviour, where changes in behaviour includes new or changed error reporting. I do not think we should report issues when the runtime is unchanged, just because there are other sources that say it shouldn't be used, even if those sources are supposed to be authoritative (e.g. the PHP documentation, RFCs, announcements, etc.).

The 'open question' from @jrfnl says:

As PHP itself only throws deprecation notices for these patterns as of PHP 8.1 and 8.3 respectively, we could consider doing the same. However, based on the description in the above mentioned RFC, the actual deprecation was as of PHP 8.0 and the fact that the deprecation notices weren't thrown for these two situations was an oversight/bug in PHP

By my reading, this is saying that PHP throws deprecation notices on PHP 8.1 and PHP >= 8.3 and therefore we should trigger the sniff for instances of this issue when the testVersion range includes those versions. This is because users will see a change in behaviour (PHP deprecation notices being thrown) in this situation. I agree with this.

I also read it as saying that PHP does not throw a deprecation in PHP 8.0 or PHP 8.2. This means there is no change in this behaviour between 7.4 and 8.0. Therefore, we should not trigger the sniff if the user is using, say, testVersion=7.2-8.0 or testVersion=8.2 as there is no problem to report.

I think what you're saying is that it was planned for this to be deprecated in 8.0 and official documentation (e.g. the RFC) says it should be deprecated in 8.0. Therefore the question being posed is "should PHPCompat also report this warning in 8.0, even though the actual PHP runtime is unchanged?", to which my answer is no, for the reasons given.

To me this seems obvious, as that is pretty much the whole point of the project. I also recall various other situations in the past where there is a discrepancy between the documentation (e.g. release notes) and actual behaviour, and previously I believe we have always followed the behaviour. Therefore, handling this as I suggest would be consistent with previous decisions.

I suggest that this pull request is merged as-is, and an issue is created here with the request to implement the required logic (which doesn't look trivial on first glance) to make the warnings only show up in the PHP versions where the corresponding notices began to be emitted.

It looks pretty trivial to me. Change line 77:

   if (ScannedCode::shouldRunOnOrAbove('8.0') === false) {

to this:

  if (ScannedCode::shouldRunOnOrAbove('8.3') === false && ScannedCode::shouldRunOn('8.1') === false) {

And then add a shouldRunOn() function, which would be trivial to implement in a similar manner to what is already there.

I don't think this should be batted to the future for technical reasons, as the coding is not difficult. It just needs a decision about which route to go down.

By my reading, this is saying that PHP throws deprecation notices on PHP 8.1 and PHP >= 8.3 and therefore we should trigger the sniff for instances of this issue when the testVersion range includes those versions. This is because users will see a change in behaviour (PHP deprecation notices being thrown) in this situation. I agree with this.

I also read it as saying that PHP does not throw a deprecation in PHP 8.0 or PHP 8.2. This means there is no change in this behaviour between 7.4 and 8.0. Therefore, we should not trigger the sniff if the user is using, say, testVersion=7.2-8.0 or testVersion=8.2 as there is no problem to report.

@MarkMaldaba Your reading of the sniff/issue is unfortunately incorrect.

PHP deprecated - via RFC - having optional parameters before required parameters in PHP 8.0.

An optional parameter is any parameter with a default value, with one (historical) exception - a parameter which is typed with a non-nullable type and a null default value.

This deprecation was implemented and released in PHP 8.0 and will show deprecation notices in PHP 8.0 and all versions above.

However, PHP Core contained two bugs in the implementation of the deprecation notice related to the historical exception:

1. Typed parameters with a nullable type and a null default value (= an optional parameter) did not throw a deprecation notice when before a required parameter.
2. Typed parameters with a union type containing null and a null default value (= an optional parameter) did not throw a deprecation notice when before a required parameter.

The first oversight was fixed in PHP 8.1 and those cases will show a deprecation notice in PHP 8.1 and all versions above.
The second oversight was fixed in PHP 8.3 and those cases will show a deprecation notice in PHP 8.3 and all versions above.

Or in code:

function not_deprecated(Foo $foo = null, $bar) {}

// All three of the below were deprecated per the RFC in PHP 8.0.
function deprecated_notice_in_80(int $foo = 1, $bar) {} // Shows deprecation notice in PHP 8.0 and up.
function deprecated_notice_in_81(?Foo $foo = null, $bar) {} // Shows deprecation notice in PHP 8.1 and up.
function deprecated_notice_in_83(int|null $foo = null, $bar) {} // Shows deprecation notice in PHP 8.3 and up.

See: https://3v4l.org/66TXZ#veol

It looks pretty trivial to me.

Nowhere near as trivial as you make it out to be as the sniff would now have to track each parameter in a function declaration to determine whether to throw the warning for PHP 8.0+, PHP 8.1+ or PHP 8.3+ as any function declaration could contain a combination of the above four syntaxes.

I believe adding additional complexity just to cover a temporary situation (given that people will at some point have to upgrade) is unnecessary.

In the end the bug in PHP was not showing the deprecation, and as a result we should assume the deprecation was always supposed to be there. As soon as people start using PHPCompatibility, PHPCompatibility will show this deprecation before the code is actually run, which is what PHP was supposed to be doing anyway. For that reason, I agree with @jrfnl

The above mentioned RFC has just passed for PHP 8.4. Given that that means we now have a fourth situation to take into account, might make sense to rewrite the whole sniff...

Closing in favour of PR #1692

@MarkMaldaba PR #1692 addresses your concerns.

The size of the diff makes it very clear how non-trivial this change was.... (+21 -3 for this PR vs +416 -58 for the new PR)

@MarkMaldaba PR #1692 addresses your concerns.

The size of the diff makes it very clear how non-trivial this change was.... (+21 -3 for this PR vs +416 -58 for the new PR)

Point taken - sorry if I introduced a bunch of extra work here. I hadn't appreciated the complexity in my original comments.

Thanks for everything you're doing - you are truly amazing!