mglaman / phpstan-drupal

Extension for PHPStan to allow analysis of Drupal code.
https://phpstan-drupal.mglaman.dev/
MIT License
196 stars 76 forks source link

EntityQuery accessCheck() false positive if chaining conditions #613

Open pfrenssen opened 1 year ago

pfrenssen commented 1 year ago

Bug report

When an EntityQuery::accessCheck() call is chained with other methods it is not detected.

This is similar to #437 but possibly requires a different fix.

Code snippet that reproduces the problem

This yields a Relying on entity queries to check access by default is deprecated error:

  $storage = \Drupal::entityTypeManager()->getStorage('node');
  $query = $storage->getQuery();
  $query
    ->accessCheck(FALSE)
    ->condition('field_myfield', TRUE);
  $entity_ids = $query->execute();

The ordering doesn't matter, if the accessCheck() is last, the error still occurs:

  $storage = \Drupal::entityTypeManager()->getStorage('node');
  $query = $storage->getQuery();
  $query
    ->condition('field_myfield', TRUE)
    ->accessCheck(FALSE);
  $entity_ids = $query->execute();

When the calls are not chained, the rule works correctly. This does not yield an error:

  $storage = \Drupal::entityTypeManager()->getStorage('node');
  $query = $storage->getQuery();
  $query->accessCheck(FALSE);
  $query->condition('field_myfield', TRUE);
  $entity_ids = $query->execute();
mglaman commented 1 year ago

Thanks for the reproducible code. I'm guessing it's due to an un-chained execute because the internal type gets lost after condition. The $query variable "resets" back to the normal PHP type.

sonnykt commented 1 year ago

I notice that chaining is okay as long as accessCheck() is followed by execute() in the chained call:

$entity_ids = $query->condition('field_myfield', TRUE)->accessCheck(FALSE)->execute();
$entity_ids = $query->accessCheck(FALSE)->condition('field_myfield', TRUE)->execute();
joachim-n commented 11 months ago

I am getting a false positive with both versions of this code:

    $query
      ->accessCheck(FALSE)
      ->condition($query->andConditionGroup()
        // The interplay between the 'status' and 'workflow' fields is not
        // clearly defined, so we expect both to be in a valid state.
        // See https://factorial-io.atlassian.net/browse/WSD-1965.
        ->condition('status', TRUE)
        ->condition('workflow', ProjectModeration::PUBLISHED)
      )
      ->sort('last_retested')
      ->pager($pager);

    $project_ids = $query->execute();

and:

    $query
      ->condition($query->andConditionGroup()
        // The interplay between the 'status' and 'workflow' fields is not
        // clearly defined, so we expect both to be in a valid state.
        // See https://factorial-io.atlassian.net/browse/WSD-1965.
        ->condition('status', TRUE)
        ->condition('workflow', ProjectModeration::PUBLISHED)
      )
      ->sort('last_retested')
      ->pager($pager)
      ->accessCheck(FALSE);

    $project_ids = $query->execute();