Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."

Issue scoping is a release management decision, not a framework decision. So consider this me removing the framework manager tag, adding the release manager tag, and then removing it again because I'm signing off on the change set. :)

The tooling giving false results means we need to review each of these on a case-by-case basis, but since in most cases the access check is executed shortly after being defined, I think it's fine to fix the ones that are definitely wrong first and then the remainder once upstream is fixed. I was able to scan the MR and confirm that there is one accessCheck() call within a few lines of the execute() within a few minutes here, so I don't think doing this in two steps creates unnecessary overhead.

We should probably file the followup first, though, and explicitly postpone it on the upstream issues.

Thanks @xjm for the explanation in #13 📌 Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." Fixed .

Putting this one back to NW to work on the follow-up and the split of this issue.

Let's postpone this on 📌 Fix failing "updated deps" test-runs by upping mglaman/phpstan-drupal to latest Fixed which fixes some false negatives and discovers some new (probably) valid errors.

📌 Fix failing "updated deps" test-runs by upping mglaman/phpstan-drupal to latest Fixed landed, back to NW.

Thanks @mallezie, must be lack of caffeine...

For me the changes look clean and good!

Only adding of access checks, and removing them from the baseline.

2 things wondering here.

There are still some entity query access checks left (in the baseline). Those might be fixed as well by a new update. Currently on mglaman/phpstan-drupal 1.1.27 (but 1.128 and 1.129 should fix some more?).
https://github.com/mglaman/phpstan-drupal/releases/tag/1.1.28
https://github.com/mglaman/phpstan-drupal/releases/tag/1.1.29

There are still some updated deps failures caused by entity access checks, which might also be fixed if we do the other update first?
See https://www.drupal.org/node/3060/qa → and https://www.drupal.org/pift-ci-job/2592021 →

So not sure if we should do another update first here?

If not this is RTBC for me.

Postponing on 📌 Fix failing "updated deps" test-runs on 10.x.x Fixed

Blocker is in. Let's try this again.

The two remaining references in the PHPStan baseline are false negatives and should be solved in mglaman/phpstan-drupal.

If I'm understanding correctly the follow up was 📌 Fix failing "updated deps" test-runs by upping mglaman/phpstan-drupal to latest Fixed

The changes in the MR look good. With no errors after being removed from the baseline think this good.

Started reviewing and 5 out of 5 of the first queries were using accessCheck(TRUE) where it should be FALSE. In practice this never makes a difference for config entity queries since no-one implements query access for them, but we shouldn't set a bad example for contrib.

Anywhere that we're checking for existence or updating should always use accessCheck(FALSE), anywhere that's listing in the UI should use accessCheck(TRUE). If it's ambiguous somewhere, we should pick one and add a comment explaining the choice.

In practice this never makes a difference for config entity queries since no-one implements query access for them

Should PHPStan(-drupal) be checking for them if that's the case?

The behaviour was only changed for content entity queries, so probably not! https://www.drupal.org/node/3201242 →

I was afraid you would say that, but it makes full sense. Back to the drawing board in mglaman/phpsta-drupal, and this is, yet another PHPStan issue that disappears in the mist of time.

It might be not that bad as thought.

https://github.com/mglaman/phpstan-drupal/issues/479

The issue is already fixed in phpstan-druoal, just waiting for a new release should be enough.

I'm sad to say it, but I find myself moving more and more away from anything PHPStan related.
There seems to be always either a policy change, a code style discussion, or a we-need-to-split-this-in-23-sub-issues involved.

Unblocked.

Spokje → closed merge request !3195

Minor formatting

(Thanks @dpi)

New approach: Not just slapping an accessCheck() on all remaining errors, but trying to fix most of them through type-hinting.

There's an open thread.

But applying the MR and searching for "^Relying on entity queries to check access by default is deprecated" there are 6 other instances.

There's an open thread.

Resolved.

But applying the MR and searching for "^Relying on entity queries to check access by default is deprecated" there are 6 other instances.

.

See IS:

As it turns out there is still a problem with mglaman/phpstan-drupal relating to accesCheck(), config entities and count queries, see https://github.com/mglaman/phpstan-drupal/issues/530.
Therefore there are still occurrences of the above errors present in the baseline. These should all be about count queries.

Back to NR.

Thanks for clarifying. All threads have been resolved. LGTM.

I was wrong about the content moderation list builder, it should be TRUE, because it's a list builder so destined for the UI, made a commit to the MR to change it back. The fact we're not getting a deprecation error from that one on tests means that method is likely completely untested (or if not something else is wrong), so tagging for a follow-up.

I also opened 📌 Deprecate node_get_recent() Fixed for the definitely-untested node_get_recent().

Committed/pushed to 10.1.x, thanks!

Spokje → closed merge request !3867

Opened 📌 Ensure ModeratedNodeListBuilder::getEntityRevisionIds has sufficient test coverage Active

Automatically closed - issue fixed for 2 weeks with no activity.