Add a filecache-backed attribute parser

Thinking through this a bit, wouldn't the ideal solution be rather than iterating over files the cache would just return the attributes with mappings back to the classes / methods needed?

I mean if we need it per file we could cache both maps.

So you might end up with two caches, one with the attributes in a given file, one with files and methods for a given attribute.

It would be good to be able to cache the file scanning more but I'm not sure we'll be able to do that more than we already do in the object cache. For example we'd have to invalidate it when modules are installed/uninstalled because the potential directories to scan will change.

So this is more about scanning a given file once for all attributes to prevent scanning a file more than once.

Re #4 yes but also the individual file scans are able to persist across drush cr - drush cr might change which files we scan, but not the contents of those files. The mtime checking and deployment_identifier keys in filecache should handle file content/logic changes.

Took a quick look at this in HookCollectorPass only, not trying to do something generic yet (and it might not be possible, or at least it wouldn't help the procedural bc layer).

I got it to the point where a drush cr doesn't explode, not sure if it actually 'works' yet.

Something that is seriously going to affect the utility of this is that the apcu cache is not available to/shared with cli requests.

I think we need a Drupal\Core\FileCache implementation that takes configuration from settings.php and can use a cache backend (but one that is not tagged for cache invalidation), or something like that. Might need a spin-off issue for that, but pushing where I got to.

Opened 📌 Add a core FileCache implementation that is database-backed Active .

Tests are passing.

This might make a difference with the UI installer, but I think we need 📌 Add a core FileCache implementation that is database-backed Active to get the real benefit of this, (and not just for this issue).

This looks pretty straightforward. Does this need dedicated test coverage to ensure it's actually using the file cache and purging the file cache properly on module install and uninstall?

Unless I am missing something this is missing the invalidation layer.
I don't see how this file gets deleted when it needs to be deleted:

As far as I can see:

• during development
• after running composer install or update ( an update or dev version might have new attributes to find)
• after install
• after uninstall

It shouldn't be necessary to purge on module install and uninstall - the module list determines discovery so any information cached for an uninstalled module will be ignored. We need add the deployment identifier to the cache key though to handle core updates.

During development the file mtime will change and that makes the cache invalid each time anyway.

Ah yes, forgot about mtime.

During development the file mtime will change and that makes the cache invalid each time anyway.

That's true!

However, I don't follow why we need to add a deployment identifier then?

However, I don't follow why we need to add a deployment identifier then?

I was thinking if the logic for collecting attributes itself changes. Maybe we don't need it though and could change the prefix directly in that case? It would make the cache work a lot better for sites that use $deployment_identifier without relying on the core version fallback.

Ah that makes sense! I'll leave it in needs work then for now.

Added in the deployment_identifier to the cache key.

PHPCS failed due to $filename not being used.

I don't want to update it so I can preserve RTBC

We can't use the Settings::get('deployment_identifier') for the file cache since it relies on the actual file to get the mtime.

Feels obvious now that you've pointed it out - the file cache depends on the filename and can't use arbitrary cids.

However, when I went to remove it, I realised we can probably add it to the namespace instead - seems like this should work but let's see what the pipeline says.

That is pretty clever!

Tagging this RTBC with the assumption we don't need to test individual cache sets and gets.

If we need that let me know.

This does have a lot of implicit coverage, if the cache wasn't storing it properly everything would break.

The original issue title was overly optimistic, maybe we can come up with a generic filecache-backed attribute parser but I think we should do a couple of one-off issues like this before that to understand the requirements better.

The changes make sense, there appears to be a further optimisation we can do but otherwise this looks good.

Good point - pushed a commit for this, since it's a trivial change, moving back to RTBC.

Fixed PHPCS, only rearranging lines and deleting some blank ones.

The "validatable config" job is failing on drush cr, unsure if it's directly related to this but looks relevant.

https://git.drupalcode.org/project/drupal/-/jobs/3366605

$ vendor/bin/drush cr --quiet
Error: Call to undefined function \ckeditor5_config_schema_info_alter() in /builds/project/drupal/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 459 #0 /builds/project/drupal/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(386): Drupal\Core\Extension\ModuleHandler->alter('config_schema_i...', Array)
#1 /builds/project/drupal/core/lib/Drupal/Core/Config/TypedConfigManager.php(406): Drupal\Core\Plugin\DefaultPluginManager->alterDefinitions(Array)

Oh that looks like we missed a direct call to a hook, I can take a look in a bit.

It could also be in drush or another vendor package.

I think that is specific to this MR and that job. The 11.x branch tip here was from before OOP hooks so it was getting very confused. I've pushed a rebase of the 11.x branch here and will re-run the job.

OK it's not that, might be to do with config_inspector but I can't see any direct hook invocations, which leaves 'something weird' or drush.

I would just require drush locally and search for ckeditor5_config_schema_info_alter. If that doesn't find it I'll investigate further when I'm at my desk in about an hour

No it is that, I just failed to push the correct branch.

Oh great! Looks good to me still and you addressed @longwave's comment on the mr.

Rtbc from me again

Let's squeeze this into 11.1.0-beta1.

Committed and pushed 6aca2acfd19 to 11.x and f6f705f8a29 to 11.1.x. Thanks!

longwave → closed merge request !10115

Reverted, this broke Experience Builder and will break many other modules that implement hooks on behalf of others.

Experience Builder implements two hooks on behalf of core modules: datetime_range_storage_prop_shape_alter and media_library_storage_prop_shape_alter - this is so the changes only take effect if those modules are installed.

During a test, we first of all install without media_library or datetime_range enabled. The hook discovery runs and these two hook implementations are not cached, because they aren't in the modules list and therefore aren't included in the function name regex. Later in the test, we install these modules, but the hook implementation cache is not cleared; the code believes it's already discovered all the hooks for experience_builder.module, the cached result is used, and the hooks are never found or executed at runtime.

This will presumably affect any module that implements hooks on behalf of other modules, the incomplete set of hooks may be cached and then never cleared. Not sure what the fix is yet, maybe we need to hash the regex as part of the cache key?

That might work, but can we also just dump it on module install?

We won't see as much benefit in that case as we'd want, but that should solve this case for now right?

catch → changed the visibility of the branch 3478621-add-a-filecache-backed to active.

Not sure what the fix is yet, maybe we need to hash the regex as part of the cache key?

The concept of implementing a hook on behalf of another module doesn't exist with OOP hooks (you can check moduleExists() inside a hook implementation of course still), so we only need to cover this with procedural hooks, so I tried implementing it only for them - seems simple enough but see what the pipeline says. The advantage if we do it this way is we only take the hit on the bc layer and only when the module list changes, so regular drush crs still get the benefit as do converted modules.

catch → reopened merge request !10115

Just a quick note, you can implement on behalf of other modules using #[Hook('toolbar_alter', module: 'module_on_behalf_of')]

You can't implement on behalf of a theme though.

Ahh #43 is a good point, however I think we're OK here because those implementations will still be discovered by the OOP attribute discovery (just not invoked when hooks are invoked), whereas the problem we have with experience_builder is the procedural implementation not being discovered at all until the module is installed. There's no way to tell which module foo_bar_foo_bar_foo_bar() is implemented on behalf of, until foo, foo_bar or foo_bar_foo module is installed, but the attribute is explicit.

These changes look good to me actually, can we test this against XB before going through the commit process again?

I created 📌 [ignore] Test Filecache Active we can apply this there

Tests are passing in XB with this change now. https://git.drupalcode.org/issue/experience_builder-3487815/-/jobs/3373246

Given we broke contrib let's add a test to ensure we don't regress again in the future