Avoid registering multiple batch sets with node_access_rebuild()'s batch mode

Fix the implementation of node_access_rebuild()'s batch mode to register only one batch set even if the function is called multiple times.

This is what I currently see in the MR.

As an alternative solution - if it turns out that there can be scenarios when running the rebuild process multiple times....

What would be those scenarios realistically? I cannot think of any at this moment... ideally there would be only one node access rebuild, triggered at the very at of the deploy process, only triggered when it is necessary.

Thanks for reporting, next step will probably be to add test coverage

Setting/detecting an arbitrary array property on the batch set feels strange, but I don't see a better way to detect whether a certain batch set is already registered, though.

One small nit on the MR, but otherwise looks like only tests are outstanding.

We could add a static in the function itself to ensure it's not added from there more than once. I guess the difference is someone could add it without calling this function.

We could add a static in the function itself to ensure it's not added from there more than once. I guess the difference is someone could add it without calling this function.

By "add it," do you mean the static variable? If the variable is regular static and not drupal_static, then it can't be accessed outside the function. If you mean adding the batch set, if someone is doing that without using this function, then the id property might not get added, either. So the static variable might be a cleaner way to go?

Alternatively, if a property in array is being used, it could be on the $batch array itself and not within the set, to save from having to loop:

// Only recalculate if the site is using a node_access module.
if (\Drupal::moduleHandler()->hasImplementations('node_grants')) {
  if ($batch_mode) {
    // Ensure that the batch set is only registered once.
    $batch = &batch_get();
    if (!empty($batch['has_node_access_rebuild'])) {
      return;
    }
    $batch['has_node_access_rebuild'] = TRUE;
    ...

Switched to using a static variable, and added a test.

I wonder if there's value in generalizing an approach to prevent certain batch sets from being added twice. My thinking is to add a static array property to BatchBuilder. Something like:

class BatchBuilder {

  protected static array $ids = [];

  public static function hasId(string $id): bool {
    return !empty(static::$ids[$id]);
  }

  public static function setId(string $id): void {
    static::$ids[$id] = TRUE;
  }

  ...
}

Then in node_access_rebuild(), it'd be something like:

if ($batch_mode && !BatchBuilder::hasId('node_access_rebuild')) {
   ...
   batch_set($batch_builder->toArray());
   BatchBuilder::setId('node_access_rebuild');
   ...

And this could be applied similarly for other uses.

ooh, I really like that. I had given thought to something more general in the batch system to prevent duplicate batches from being set, but I was thinking about thinks like having batch_set hash the definition array and preventing duplicates that way, but it would be a behavior change, and I can't be certain that someone doesn't have. a legitimate reason to duplicate a batch somewhere for some reason, so I kept the focus on the rebuild.

This is a simple and elegant helper method in the helper class for code to ensure it doesn't add a duplicate batch if it doesn't want to.

MR looks good. I have 1 minor comment, but it's personal code style preference, and I don't feel that strongly about it.

Thanks for the improvements! It's definitely much better now than my original fix. I was also thinking about a more general solution but I didn't want to touch the other parts of the system.

Awesome improvements indeed! Maybe announcing the new API worth a change record?

CR added: https://www.drupal.org/node/3512253 → (@mikelutz provided helpful docblock that could be copied).

Note to update the CR if it's decided that method names need changing.

NW to bikeshed method names. :-)

Fixed/tweaked a few docs while I was in there, setting back to NR

lgtm

Looks good. Committed/pushed to 11.x, thanks!

catch → closed merge request !11384

Is this eligible for backporting?

Yes - cherry-picked to 10.5.x, thanks!

Thank you, too!