Problem/Motivation

With the version bump of mglaman/phpstan-drupal in #3330305: Fix failing "updated deps" test-runs by upping mglaman/phpstan-drupal to latest some Entity Query access check-related errors popped up and were committed to the baseline core/phpstan-baseline.neon.

This issue aims to fix all those errors, which manifest themselves as:

Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked.
💡 See https://www.drupal.org/node/3201242

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.

Steps to reproduce

Proposed resolution

- There should be no more of the above mentioned errors being suppressed in the baseline.
- TestBot should return green.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3330981

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Fix PHPStan L2 error "Cannot call method Foo() on Bar." » Fix PHPStan L2 error "Relying on entity queries to check access by default is deprecated..."
spokje’s picture

spokje
Primary language Dutch
commented
Title: Fix PHPStan L2 error "Relying on entity queries to check access by default is deprecated..." » Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."

Spokje opened merge request !3194

Spokje closed merge request !3194

spokje’s picture

spokje
Primary language Dutch
commented
Version: 10.0.x-dev » 10.1.x-dev

Spokje opened merge request !3195

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Active » Needs review

Unsure how to proceed.

There are a number of false positives/negatives, for which I filed upstream issues https://github.com/mglaman/phpstan-drupal/issues/494 and https://github.com/mglaman/phpstan-drupal/issues/496.

Do we want to wait until those are fixed and released, or do we want to get the ones that are solid in now and do the rest later?

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Removing credit from myself as I rebase the MR to see if there are failures.

Appears it does have one

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @smustgrave, unsure how this could have passed in previous runs.

Back to NR for #8.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative, +Needs framework manager review

Discussed this one in slack with @Spokje

there are false positives/negatives which need an upstream mglaman/phpstan-drupal fixes.
I've created a few PRs over at their Git.
Each false positive/negative has a comment in the MR itself (https://git.drupalcode.org/project/drupal/-/merge_requests/3195#note_144060 and downwards) linking it to one of those PRs.

So the question for the framework managers are do we fix some now and open a postponed follow up for the others later. Or postpone this until the phpstan-drupal fixes are in?

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs framework manager review +Needs followup

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.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work

Thanks @xjm for the explanation in #13.

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

spokje’s picture

spokje
Primary language Dutch
commented
Title: Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." » [PP-1] Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."
Status: Needs work » Postponed

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

spokje’s picture

spokje
Primary language Dutch
commented
Status: Postponed » Needs work

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

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented
Title: [PP-1] Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." » Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."
spokje’s picture

spokje
Primary language Dutch
commented

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

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented

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.

Seems i'm crossposting while Spokje still working on it. Ignore my code comments then for now.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." » [PP-1] Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."
Status: Needs work » Postponed
Related issues: +#3341254: Fix failing "updated deps" test-runs on 10.x.x, +#3336586: Fix failing "updated deps" test-runs by upping mglaman/phpstan-drupal to latest

Postponing on #3341254: Fix failing "updated deps" test-runs on 10.x.x

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented
Title: [PP-1] Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." » Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."
Status: Postponed » Needs work

Blocker is in. Let's try this again.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

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

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

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.

spokje’s picture

spokje
Primary language Dutch
commented

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?

catch’s picture

catch
he/him
Primary language English
commented

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

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Postponed

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.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." » [PP-upstream] Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."
mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented

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.

spokje’s picture

spokje
Primary language Dutch
commented

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.

spokje’s picture

spokje
Primary language Dutch
commented
Title: [PP-upstream] Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..." » Fix PHPStan L1 error "Relying on entity queries to check access by default is deprecated..."
Assigned: Unassigned » spokje
Status: Postponed » Needs work

Unblocked.

Spokje closed merge request !3195

Spokje opened merge request !3867

dpi’s picture

dpi
Location Perth, Australia
commented
Issue summary: View changes

Minor formatting

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented

(Thanks @dpi)

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

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

  • catch committed c49ae937 on 10.1.x 
    Issue #3330981 by Spokje, smustgrave, mallezie, catch, xjm: Fix PHPStan...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs follow-up

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 #3356516: Deprecate node_get_recent() for the definitely-untested node_get_recent().

Committed/pushed to 10.1.x, thanks!

Spokje closed merge request !3867

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: -Needs follow-up

Opened #3357071: Ensure ModeratedNodeListBuilder::getEntityRevisionIds has sufficient test coverage

Status: Fixed » Closed (fixed)

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