Add [#NoDiscard] to AccessResult methods or make them chainable

Sorry no harm in being review but don't want to accidentally get reviewed. #3575572: Add [#NoDiscard] to Utility classes is determining if this is worth it.

Here the reason is clear - even exposing a bug

+1 to fixing this given that it's found a genuine bug. Technically this is a security issue but it appears to be such an edge case - the user has access to view all revisions, and is viewing a non-default revision, and doesn't have access to view the original revision - that I think this is better just to fix here.

FWIW I think this should be backported to 10.6 as well, bumping priority too.

+1 for RTBC.

If we backport this issue to 11.3 and 10.6, then we should not add #[NoDiscard] since it can generate errors, flooding logs and causing automated tests to fail.

Committed a94c396 and pushed to main and
committed bcfde83 and pushed to 11.x.

Re: #11: we have symfony/polyfill-php85 on 11.3.x, which is documented to support NoDiscard, but I have seen PHPStan complain on array_all or array_any on a build, even with the polyfill. I'll see what the 11.x 8.3 pipelines look like before pushing.

This will need a backport for 10.6.x with NoDiscard removed, though.

In Comment #11, I should have said "warnings", not "errors". Warnings are enough to flood logs and to break automated tests and related CI.

@godotislate, I do not see how having a polyfill affects these reasons not to add #[NoDiscard] in a patch release.

I assume you mean the warnings about the attribute not being defined in PHP < 8.5?

ETA. I'm fine with an MR to remove the attributes for 11.3.x as well.

I think @benjifisher means the warnings are a behaviour change for anyone who is currently accessing these APIs incorrectly, it might start triggering messages in their logs or CI etc. It is okay to start warning these users in the next minor but we should not do it in a patch release.

@godotislate:

From https://www.php.net/NoDiscard:

This attribute can be used to indicate that the return value of a function or a method should not be discarded. If the return value is not used in any way, a warning will be emitted.

Testing with PHP 8.5.2:

<?php
// file foo.php

#[\NoDiscard()]
function foo(): int {
  return 17;
}

then

$ php -a
Interactive shell

php > require 'foo.php';
php > $n = foo();
php > echo $n;
17
php > foo();
PHP Warning:  The return value of function foo() should either be used or intentionally ignored by casting it as (void) in php shell code on line 1

Warning: The return value of function foo() should either be used or intentionally ignored by casting it as (void) in php shell code on line 1

@benjifisher: heard.

I thought you were talking about 11.3.x or 10.6.x CI builds running on PHP 8.3 or 8.4 and failing because of PHPStan flagging NoDiscard as an undefined attribute. It didn't occur to me that hose builds could be running on PHP 8.5.

@catch: @goditislate already committed the fix to the main and 11.x branches, so I am updating the branch to 11.3.x.

I prepared MR !15196 for the 11.3.x branch. As I said in Comment #11, we should not back-port the #[NoDiscard] annotation to patch releases. The commit should cherry-pick cleanly to the other supported branches.

I needed some help to get the tests to run, but now they are all green, so back to NR.

Thank you @benjifisher for the port. It also applied clean to 10.6.x.

Committed 5c02b60 and pushed to 11.3.x and committed 4b3b0b4 and pushed to 10.6.x. Thanks!