🇺🇸United States @mikeryan

mikeryan → created an issue.

While we're here - what about setCacheKey() and writeCache()? These public methods are:

1. Implemented in AliasManager
2. Not defined in AliasManagerInterface
3. Used in PathAliasSubscriber via a member defined as an instance of AliasManagerInterface

In other words, if you were to implement your own alias manager using the interface without referencing the internals of the core implementation - it would blow up.

No, I'm not actually trying to implement my own alias manager - I'm decorating AliasManager, and phpstan is (rightfully) unhappy with the situation:

  32     Method Drupal\me_performance\MePerformanceAliasManager::setCacheKey() has parameter $key with no type specified.
  33     Call to an undefined method Drupal\path_alias\AliasManagerInterface::setCacheKey().
  40     Call to an undefined method Drupal\path_alias\AliasManagerInterface::writeCache().
  53     Method Drupal\me_performance\MePerformanceAliasManager::cacheClear() has parameter $source with no type specified.

These methods were originally defined in `CacheDecoratorInterface` - when that was deprecated → , the methods probably should have been moved to `AliasManagerInterface`.

FWIW, I tried patching `acquia_search` with the `reloadCore()` implementation from `StandardSolrCloudConnector`, but it failed with an authentication error from `/solr/admin/collections` - I guess the platform simply doesn't permit that level of access?

mikeryan → created an issue.

xjm → credited mikeryan → .

The key point from the issue description is "Response code 403, 404 excluded" - we have the exact same situation. The problem is simply that the ResponseCode condition plugin is not applying negation. Patch attached.

@mstrelan Thanks! assert() did the job nicely...

We're having CI failures on our app with Drupal 10.2.4, which passes under Drupal 10.2.3, and this appears to be the relevant change.

Error: Cannot access offset 'id' on array|Drupal\Component\Plugin\Definition\PluginDefinitionInterface.
 ------ --------------------------------------------------------------------- 
  Line   src/ActiveSitePluginBase.php                                         
 ------ --------------------------------------------------------------------- 
  54     Cannot access offset 'id' on                                         
         array|Drupal\Component\Plugin\Definition\PluginDefinitionInterface.  
 ------ --------------------------------------------------------------------- 

The triggering code:

  /**
   * {@inheritdoc}
   */
  public function id(): string {
    return $this->pluginDefinition['id'];
  }

This should still be correct usage, shouldn't it? Core is still full of instances of $this->pluginDefinition['key'].

mikeryan → created an issue.

Here it is!

mikeryan → created an issue.

Note that there are at least two contrib modules (just in our current project) that will need changes to avoid "Declaration must be compatible with Drupal\layout_builder\EventSubscriber\SetInlineBlockDependency::getInlineBlockDependency(Drupal\block_content\BlockContentInterface $block_content, string $operation): ?Drupal\Core\Access\AccessibleInterface":

• Layout Builder Asymmetric Translation → (patch at https://www.drupal.org/project/layout_builder_at/issues/3090261 🐛 SetInlineBlockDependency override might no longer be needed. Needs work )
• Workspaces Extra → (no patch yet)

Adding the related issue https://www.drupal.org/project/drupal/issues/3047022 🐛 Layout builder fails to assign inline block access dependencies for the overrides section storage on entities with pending revisions Needs work - we're also running into this when trying to use that patch.

Yep, what's done is done. Good motivation for us to do as you did here and get away from old-school inheritance...

Yes, good practice. But this is a breaking change for applications which overrode the now-absent constructors - it would have been nice to bump the major version in this case.

Tests pass - guess I didn't break anything!

mikeryan → created an issue.

Needs reroll.

Botched the last patch, here's a fresh one.

A tweak - record failed updates so they don't keep getting retried.

We're using this now in our project, just one problem identified so far - on a fresh install, these "hookless updates" are not recorded in existing_updates, so the first drush updb will run them (which it shouldn't). This is addressed by adding them to existing_updates in hook_preinstall().

OK, the only change to the tests for them to pass with my changes present but disabled was to add the extra dependency to `ConfigHandler`. It'd be nice if we could avoid that...

Tweaks to make CI happier.

Some tweaks, but a little problem identified. To have our hook implementing the hookless updates run every time, we remove it from existing_updates in hook_cache_flush(). However, on initial site install, our hook is recorded in existing_updates, and update_helper_cache_flush() is never called, so if you run drush updb -y our hook does not run. There needs to be an intervening drush cr for it to work. I tried removing the reference in hook_install(), but it's too early.

Relatedly, initially it took two or three drush crs to get updb to work. It seems that on install existing hooks get recorded in existing_updates multiple times (so simply doing array_search and removing the instance found isn't enough). I addressed it here with

$existingUpdates = array_unique($existingUpdates);

In my case, count($existingUpdates) went from 570 to 190.

This knocks a couple of items off the remaining tasks - there's a config option (no UI, I think as a dev option settings.php is fine, at least for now), and we make sure we don't short-circuit config files with explicit hooks. Example: Without that fix, the xmlsitemap update config change would have been run by update_helper, and xmlsitemap_post_update_0001_zzzzzzz() would not have run, which would be a bad thing if it had anything in addition to the config update:

mikeryan@meglobal-web:/var/www/html$ drush updb -y
 --------------- --------------------------- ------------- ----------------------------------------------
  Module          Update ID                   Type          Description
 --------------- --------------------------- ------------- ----------------------------------------------
  update_helper   9999_run_hookless_updates   post-update   Process config updates not invoked in hooks.
  xmlsitemap      0001_zzzzzzz                post-update   Configuration update.
 --------------- --------------------------- ------------- ----------------------------------------------


 // Do you wish to run the specified pending updates?: yes.

>  [notice] Update started: update_helper_post_update_9999_run_hookless_updates
>  [notice] Update completed: update_helper_post_update_9999_run_hookless_updates
>  [notice] Update started: xmlsitemap_post_update_0001_zzzzzzz
> [info] Configuration xmlsitemap.settings has been successfully updated.
>  [notice] Update completed: xmlsitemap_post_update_0001_zzzzzzz
 [success] Finished performing updates.

A point that was in the back of my mind, and made explicit by a colleague, is that there's still the danger of parallel updates conflicting - this support would make it harder to recognize the problem early.

1. Developer 1, working on ticket ABC-123, implements example_post_update_0008_abc_123().
2. Developer 2, working on ticket ABC-124, implements example_post_update_0008_abc_124().
3. Developer 1's pull request is merged.
4. Developer 2's pull request is merged.
5. If the two PRs do not touch the same configuration, everything's fine.
6. However, if they do touch the same configuration, consider that in the consuming project dev 1's update will run first, then dev 2's will run on top of it even though it has been defined as a diff from the original configuration, not from the configuration resulting from the first update. In the sense that it may require Developer 2 to reroll, it's seemingly no worse than how things work today - but unlike today's scenario, it won't be immediately obvious after Dev 1's merge. It may or may not cause errors when Dev 2's update is run - or it may result in some problem more subtle (like overwriting some of Dev 1's changes). This would be difficult to diagnose.

In our particular workflow, this may be an acceptable risk, so we may apply my patch from here. However, unless someone comes up with a good answer to the last point, I'll certainly understand if the update_helper maintainers aren't interested in accepting this patch.

Just added to remaining tasks:

How to ensure this is the last post_update hook run? UpdateRegistry::getAvailableUpdateFunctions() sorts by function name, so update_help hooks are going to be *towards* the end, but not guaranteed to be last.

So, if module xxx has a post_update config update in config/update, and has an explicit hook to execute it in xxx.post_update.php, our support here will apply the update before xxx's hook has a chance. And mark it as done, so it will be skipped. Which... is only a problem if any needed additional work was explicitly added to that hook. It's a problem likely to be very rare in practice, but if it does happen it'll be *really* hard for the victim to figure out why.

Actually, though, thinking as I type, we could (and should) avoid that by excluding in runHooklessUpdates() not just updates already run, but updates corresponding to existing functions... Thanks for being my rubber duck!

Obviously, existing tests need to be updated, as well as adding tests for the new functionality...

Oops...

Setting "Needs review" just so tests can run...

Figured out a way to run the updates without a separate Drush command. Some tweaking to do, obviously needs tests, but here's a sample session of three steps:

1. Generate the update (creates the .yml file, does not touch the post_update.yml file).
2. Run updates - it applies the config from the newly-generated file.
3. Run updates again - nothing is applied.

$ drush generate update_helper:configuration-update
 [warning] Undefined array key "no-ansi" GenerateCommands.php:76

 Welcome to config-update generator!
–––––––––––––––––––––––––––––––––––––

 Enter a module/profile:
 ➤ me_group

 Do you want to create a post_update or hook_update_N configuration update? [post_update]:
  [0] post_update
  [1] hook_update_N
 ➤ 0

 Do you want to create a hook function for the update? [yes]:
  [0] yes
  [1] no
 ➤ 1

 Please enter the machine name for the update:
 ➤ test_123

 Provide a comma-separated list of modules which configurations should be included in update.:
 ...
  [ 65] me_group
...
➤ 65

 Generate update from active configuration in database to configuration in Yml files? [Yes]:
 ➤

 The following directories and files have been created or updated:
–––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
 • /var/.../me_group/config/update/me_group_post_update_0006_test_123.yml
$ drush updb -y
 --------------- --------------------------- ------------- ----------------------------------------------
  Module          Update ID                   Type          Description
 --------------- --------------------------- ------------- ----------------------------------------------
  update_helper   9999_run_hookless_updates   post-update   Process config updates not invoked in hooks.
 --------------- --------------------------- ------------- ----------------------------------------------


 // Do you wish to run the specified pending updates?: yes.

>  [notice] Update started: update_helper_post_update_9999_run_hookless_updates
> [info] Configuration core.entity_form_display.group.content.default has been successfully updated.
> [info] Configuration core.entity_view_display.group.content.default has been successfully updated.
> [info] Configuration update me_group_post_update_0006_test_123 applied
>  [notice] Update completed: update_helper_post_update_9999_run_hookless_updates
 [success] Finished performing updates.
mikeryan@meglobal-web:/var/www/html$ drush updb -y
 --------------- --------------------------- ------------- ----------------------------------------------
  Module          Update ID                   Type          Description
 --------------- --------------------------- ------------- ----------------------------------------------
  update_helper   9999_run_hookless_updates   post-update   Process config updates not invoked in hooks.
 --------------- --------------------------- ------------- ----------------------------------------------


 // Do you wish to run the specified pending updates?: yes.

>  [notice] Update started: update_helper_post_update_9999_run_hookless_updates
>  [notice] Update completed: update_helper_post_update_9999_run_hookless_updates
 [success] Finished performing updates.

A little annoying that the post-update hook always shows, whether it has anything to do or not.

First draft of part 1 of the resolution (skip hook generation). The directory scan should probably be moved to ConfigHandler, but this will serve as a proof-of-concept.

mikeryan → created an issue.

Restoring "Needs review", since my D7 testing bumped it back to "Needs work"...

Oops...

Not that anyone asked for this... But I'm working on a D7 project now that needs this, so here's a reroll of #42.