Problem/Motivation

Upgrade status module reports a warning:
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.

Line 374 - web/modules/contrib/flippy/src/FlippyPager.php

Steps to reproduce

Install latest version of module (2.0.0-beta0) and enable
Run Drupal 10 upgrade status and scan flippy module
Error is listed on D10 compatibility report

Proposed resolution

Add access check to random list function.

CommentFileSizeAuthor
#2 access-check-missing-3383228-2.patch409 byteswylbur

Issue fork flippy-3383228

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:

Comments

wylbur created an issue. See original summary.

wylbur’s picture

StatusFileSize
new409 bytes

Here's a patch that resolves the D10 warning for missing access check.

keshavv’s picture

Status: Active » Needs review
anybody’s picture

Assigned: wylbur » grevil
Status: Needs review » Needs work

Guess this should be TRUE, not FALSE ;)

anybody’s picture

Assigned: grevil » anybody

No idea what that patch does :D Not even the variable exists?

Edit: Sorry I was wrong, the variable exists ;)

anybody’s picture

Assigned: anybody » grevil
Status: Needs work » Needs review

@Grevil: Please review

grevil’s picture

Status: Needs review » Needs work

Seems odd to me... I guess we need to also add the access check for:

      $first = clone $query;
      $prev = clone $query;
      $next = clone $query;
      $last = clone $query;

But I don't get, why we can not simply use the access check of the original query... oh well.

anybody’s picture

@Grevil indeed sorry! Yes the same question came to my mind... perhaps because it's a method? We shouldn't dig too deep?
I would remove the whole clone in a refactoring, I think. Quite risky.

grevil’s picture

I agree, I don't even think it is possible to add an access check later on to a populated query. So removing the clone is probably our only option here... A bit unfortunate, since we create duplicate code this way...

grevil’s picture

On further inspection, this kind of seems like a core bug. As stated in https://www.drupal.org/node/3201242, the warning

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.

Should only appear if accessCheck() is called without any parameters given.

But in this case, we explicitly set 'accessCheck(TRUE)' on the original query and clone the query object 4 times, meaning the cloned object should also have their accessCheck set explicitly!

grevil’s picture

Status: Needs work » Postponed (maintainer needs more info)

I just reread the issue summary and found out, that this isn't an actual error, but instead output by upgrade status...

Have you actually seen the error get thrown using Drupal 10? My guess is, that the upgrade status module isn't smart enough to see, that the explicit access check was simply cloned from the original query object.

grevil’s picture

Assigned: grevil » Unassigned