Problem/Motivation
There is a long standing issue (see here) when accessing config from a hook_install().
In some scenarios (in my case, after upgrading to Drupal 10.1.x and running drush site-install --existing-config), the config is not ready when executing hook_install, so this will fail as the 'authenticated' role will return NULL:
$role = Role::load('authenticated');
Steps to reproduce
Perform drush site-install --existing-config on an existing site that has Matomo module installed using Drupal 10.1.x.
Proposed resolution
Maybe the hook_install code should belong to a previous step (deploy hook? hook_post_config_import?) where we are sure that Drupal config is completely imported and present.
Remaining tasks
TBD
User interface changes
None
API changes
None
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | matomo-3363521-7.patch | 1.65 KB | grimreaper |
Issue fork matomo-3363521
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3363521-error-on-hookinstall
changes, plain diff MR !44
Comments
Comment #3
idiaz.ronceroFor those in need of a quick workaround, I've created a MR that simply bypasses the code when there is no Role loaded.
This is clearly NOT the final solution, as it will result on divergent results (i.e: after a
drush site-install --existing-configyour permissions might be different as hook_install won't be executed!)This is just a workaround for those that need to have a temporary solution. I keep the issue as "Needs work"
Comment #4
ressaI got this error, after installing a minimal install and then running
drush site:install --existing-config -yas shared by Philip Norton in Drupal 8: Install Site From Existing Configuration in Drupal 10.1.As a temporary workaround I also commented out those lines during the install + import of config, after which it completed.
Comment #5
ressaThis breaks deployment, so upping Priority.
Comment #6
grimreaperComment #7
grimreaperHi,
After trying to update one of my website on Drupal 10.1 and reinstalling I also get this issue. And a similar issue for other contrib modules (Fontawesome).
Thanks for the MR. Attaching a patch file from it for Composer usage.
I agree that this is only a workaround and not a final solution. I currently do not have the time to search for a definitive solution.
Maybe switch to hook_modules_installed? adding a dependency on the user module (I tried that for Fontawesome, and it was not a success or I tested too quickly)?
I am in favor of merging and making a new release to unblock people.
@shelane, what do you think?
Comment #8
rosielIn a similar issue on a different module, @neclimdul proposed using a helper function,user_role_grant_permissions. See https://www.drupal.org/project/userprotect/issues/3349663#comment-15104566Edit: That appears to not work.
Comment #9
seanbIn #3349663-7: Error: Call to a member function grantPermission() on null in userprotect_install() neclimdul found out what is happening. In short, this is caused by the change in #3112866: Remove special case of User module install. Modules are installed first and the default config for the modules is imported later. Since the authenticated user role is default config of the user module, the role doesn't exist yet in the install hook of other modules.
We can add the permissions using
user_role_grant_permissions()which is also used in core bymedia_install()andnode_install(). It basically uses the same checks as the patch, so we don't have to duplicate that code. We should probably also add a check for the user module existance (or add a dependency on the user module, but that seems a bit silly).Since this only seems to happen when installing sites from existing config, it shouldn't really matter that the module install hook can't add the permissions to the authenticated user role.
Comment #10
grimreaperHi,
Thanks @seanB for the explanations and suggestions.
I have updated the MR.
If people are ok with it I will merge and create a new release when I will have time.
Comment #11
emek commentedWe have tried a patch with the latest changes in the MR and now installation works for us.
Comment #12
bircherWe are using a patch created from this MR (at the time of writing d74c9750)
This can be merged for what I can tell.
Comment #13
ressaThanks for the patch, it works well and fixes the bug, and can be rolled out.
Comment #14
grimreaperComment #17
grimreaperHi,
Merged manually on 8.x-1.x because of Gitlab CI workflow not ok. I guess due to dependency on PHP module.
Cherry-picked on 2.0.x.
Creating a new release for 8.x-1.x
Comment #18
shelaneThank you @Grimreaper!
Comment #19
ressaThanks for the new release, it's always nice to have one less patch file and entry in composer.json to juggle with :)