Update mglaman/phpstan-drupal

Looks mglaman/phpstan-drupal:1.1.16 like came with a new "Missing explicit access check on entity query." check.

Let's try to fix the new suppressions for that in here.

@mallezie:

do you mean clean the access checks in this issue? Or do a followup?

In here, like you also suggesting :)
I think I can get those fixed within the next couple of hours, which is (IMHO) good enough for even a Critical?
If not: Let me know and we'll fix it in a follow-up.

This got into the composer update (probably because spokje's compers version is higher then the one from drupal core).

-    "plugin-api-version": "2.2.0"
+    "plugin-api-version": "2.3.0"

Indeed my mistake, not sure the policy, but I've reverted it whilst working on solving the access checks.

Thanks, I kinda like non-critical criticals... 😇

Ok, so this release brings _a lot_ of false positives on Missing explicit access check on entity query.
(See comments in MR, https://github.com/mglaman/phpstan-drupal/issues/396 and https://drupal.slack.com/archives/C033S2JUMLJ/p1652274305914599?thread_t...)

If (and only if) this is all this release brings to the table, I would *not* recommend shipping it with drupal/core, but rather wait on a release that has much less noise.

However, I think this release also would considerably decrease mem-usage when doing a full file run. If this is the case, I would say: Use the current MR, leave all new suppressions in the baseline to make #3259355: Always do a full phpstan analysis on DrupalCI happen, and deal with the false positives when a new release comes out (which hopefully has less of those).

Putting this on NR to get more eyes/brains/random-body-parts on this.

Thanks @mallezie, was just doing the required reroll as you were typing.

IMO i would say commit this (but needs reroll, since the out-of-sync-baseline issues are in). Add those items to the baseline.
Then postpone the access checks cleanup from the baseline to a follow-up phpstan-drupal release which might fix more false-positives.

I can live with that :)

Opened #3280071: [PP-1 Upstream] Clean-up (if still around) false positive on PHPStan "Missing explicit access check on entity query." for the (hoepfully not needed) cleanup of the baseline when a new version is released

(Opened https://github.com/mglaman/phpstan-drupal/issues/406 for the Windows vs POSIX issue that now bit me twice)

Reroll managed to scrape by The Land Of JSTestFailure-Doom. Needs Review

Thanks @mallezie.

Just realized the fix (and the fix only) should be backported down to 9.3.x-dev. Adding patch for that.

This can't be a critical issue. It does not meet any of the requirements for one laid here - https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

Also I think we should fix the broken code in a way that does less. Let's make people supply the title - they have to do that today because it is broken - and we don't know that because we don't use it.

Also the fix to \Drupal\Tests\quickedit\FunctionalJavascript\SettingsTrayIntegrationTest::createBlockContent() needs to be backported to 9.x

Also I think we should fix the broken code in a way that does less. Let's make people supply the title - they have to do that today because it is broken - and we don't know that because we don't use it.

Changing patch/MR.

Also the fix to \Drupal\Tests\quickedit\FunctionalJavascript\SettingsTrayIntegrationTest::createBlockContent() needs to be backported to 9.x

You might have missed the patch in #15 which does just that?
Anyway, that will be adapted to the solution in your first quote.

Also this needs to be backported to the Contrib QuickEdit, for which I already created #3280145: Invalid/outdated code in \Drupal\Tests\quickedit\FunctionalJavascript\SettingsTrayIntegrationTest which I will reroll _after_ we have a commit in this issue

Thanks @Spokje that looks great.

When I tried to commit this I got the following reported:

Note: Using configuration file /Volumes/dev/drupal/core/phpstan.neon.dist.
 9191/9191 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   core/modules/node/node.module
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------
  1090   Ignored error pattern #^Missing explicit access check on entity query\.$# in path /Volumes/dev/drupal/core/modules/node/node.module is expected to occur 3 times,
         but occurred only 1 time.
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------



 [ERROR] Found 1 error

The odd thing is that the node.module actually has 2 entity queries missing the access check - not 1 or 3. They are both in the completely unused in core node_get_recent(). I think the entity query access checking check must be flawed and I think either we have to disable the rule and get it fixed upstream or we have to get it fixed upstream and then use that new version here.

If I remove the error from the baseline completely and re-run phpstan then it tells me...

Note: Using configuration file /Volumes/dev/drupal/core/phpstan.neon.dist.
 9191/9191 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------
  Line   core/modules/node/node.module
 ------ ------------------------------------------------
  1090   Missing explicit access check on entity query.
         💡 See https://www.drupal.org/node/3201242
 ------ ------------------------------------------------



 [ERROR] Found 1 error

However here's the lines from node.module around 1090...

      $entity_query = \Drupal::entityQuery('node');
      $entity_query->sort('nid', 'DESC');
      // Disable access checking since all nodes must be processed even if the
      // user does not have access. And unless the current user has the bypass
      // node access permission, no nodes are accessible since the grants have
      // just been deleted.
      $entity_query->accessCheck(FALSE);
      $nids = $entity_query->execute();

So that query is doing it correctly.

I decided to commit #22 to all branches since that is fixing something that is broken and needs a fix in more places than the update and opening a separate issue for it felt meh.

Committed and pushed ce7120d598 to 10.0.x and f88b95826e to 9.5.x and 9af805fa84 to 9.4.x. Thanks!

Some clean-up after the commits above.

Opened #3280328: Stop ignoring "Missing explicit access check on entity query." rule in core/phpstan-baseline.neon as a follow-up

Ignoring the questionable rule while at the same time bumping the dependency seems to me a sensible approach. https://drupal.slack.com/archives/C1BMUQ9U6/p1652431244395749 for background.

We remove false positives from the baseline thanks to better checks implemented in the latest dependency releases, so all seems good here.

Committed 33de6ee and pushed to 10.0.x. Thanks!