Problem/Motivation

We're talking at https://www.drupal.org/project/drupal/issues/106721#comment-14115502 and on the file
core/modules/node/src/NodeGrantDatabaseStorage.php
 there is a method buildGrantsQueryCondition that we can use early return to be easier to ready and avoid Hadouken effect

Proposed resolution

Change from:

protected function buildGrantsQueryCondition(array $node_access_grants) {
  $grants = $this->database->condition('OR');
  foreach ($node_access_grants as $realm => $gids) {
    if (!empty($gids)) {
      $and = $this->database->condition('AND');
      $grants->condition($and
        ->condition('gid', $gids, 'IN')
        ->condition('realm', $realm)
      );
    }
  }

  return $grants;
}

To:

protected function buildGrantsQueryCondition(array $node_access_grants) {
  $grants = $this->database->condition('OR');
  foreach ($node_access_grants as $realm => $gids) {

    if (empty($gids)) {
      continue;
    }

    $and = $this->database->condition('AND');
    $grants->condition($and
      ->condition('gid', $gids, 'IN')
      ->condition('realm', $realm)
    );
  }
  

  return $grants;
}
CommentFileSizeAuthor
#4 3216057-4.patch985 bytesrenatog
#3 3216057-3.patch739 bytesrenatog
#2 3216057.patch167 bytesrenatog

Comments

RenatoG created an issue. See original summary.

renatog’s picture

renatog commented
Status: Active » Needs review
StatusFileSize
new 3216057.patch167 bytes

This is the patch with the fix

renatog’s picture

renatog commented
StatusFileSize
new 3216057-3.patch739 bytes

This is the correct patch

renatog’s picture

renatog commented
StatusFileSize
new 3216057-4.patch985 bytes

Please discard the other patches. This is the correct patch

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Status: Needs review » Reviewed & tested by the community

Nice implementation @RenatoG!

Moving to RTBC!

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Assigned: Unassigned » diegors
Status: Reviewed & tested by the community » Needs work

Tests are failing in Drupal 9.2.x, I will work on it.

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Assigned: diegors » Unassigned
Status: Needs work » Reviewed & tested by the community

As this patch apply successfully in Drupal 9.2.x and 9.5.x, and implements an early return, for me makes sense to keep in RTBC, if someone disagrees please move back.

bhanu951’s picture

bhanu951 commented
Version: 9.5.x-dev » 10.1.x-dev

Changes should be applied to latest Head and Back-ported. Hence updating Version.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Closed (won't fix)

There's no performance benefit from this change and the readability benefits are debatable. If the code inside the if (!empty($gids)) { was large then maybe this would be more readable. Also the issue is not titled correctly we're not doing an early return - we're using continue in a foreach loop. I think committing this patch would set us up for lots of similar issues which would take time to implement, review and commit for little to no benefit.

Discussed with @catch we agreed to close this.