Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When the module is install during config import it generates error because uuid of file is not present. There is a work around in core to fix that see forum_install()
. Following patch fixed that.
Comment | File | Size | Author |
---|---|---|---|
#61 | fix_the_module_install-2696593-61.patch | 3.65 KB | samuel.mortenson |
| |||
#61 | fix_the_module_install-2696593-61-test-only.patch | 1.3 KB | samuel.mortenson |
#57 | fix_the_module_install-2696593-57.patch | 2.4 KB | heddn |
| |||
#54 | fix_the_module_install-2696593-54.patch | 1.95 KB | jibran |
|
Comments
Comment #2
jibranRelated core issue #2550385: Update hook_install documentation for editing configuration.
Comment #3
DamienGR CreditAttribution: DamienGR commentedThank you jibran, it works.
Comment #4
jibranRTBC maybe?
Comment #5
samuel.mortensonWould this change mean that the Entity Embed button will not have an icon if the module is installed during config import? We always want an icon to be present for our Entity Embed button, so if there's a specific error we can address instead of skipping the install logic I would prefer that.
Comment #6
jibran@samuel.mortenson I just verified this on the client staging site and this is correct. I pinged @alexpott about this.
Comment #7
jibranComment #8
samuel.mortensonOk, with that in mind I think the most complete solution would be to have this check, and have an update hook to be run post-config sync that checks if the icon is added, and if not adds it. Does that make sense?
Comment #9
jibranYeah, that makes sense.
Comment #10
jibranWe can set some variable in state during hook_install and in post update hook if the variable is set we'll add the icon and unset the the variable.
Comment #11
jibranHow about this?
Comment #12
alexpottThis is run as part of update.php and behaves like hook_update_N(). I think all you need to do is check if the config is syncing. If the incoming going has the module installed then the incoming config should also have the uuid set.
Also the
embed.button.file_browser
should not fail importing if the file entity is not present - all dependencies on content should be soft and not enforced during import in order to prevent issues like this.Comment #13
jibranI created the first patch https://www.drupal.org/files/issues/fix-install_0.patch and it was doing the exactly same thing and we ran into #5 because of that.
It imports the config just fine but it
$uuid = $file->uuid();
generates error trying to access uuid method on NULL.Comment #14
samuel.mortenson#13 covers it, the icon is not required for the module to function - but is a "nice to have"/soft dependency and as far as I can tell this sort of install routine is the only way to add an icon to an Entity Embed button.
Comment #15
samuel.mortenson@jibran Is this missing an include/require/info.yml change? Also, would the file_browser_post_update_set_file_browser_icon hook be run every time an update is ran? I want to provide a default icon on install, but also want users to be able to change that icon if they want to without overriding it later on.
Comment #16
jibranNo patch is complete.
No it'll just run once.
This is not run on install now. After the install you have to run update.php to setup the icon.
Comment #17
alexpottThe current patch is not right... all you need to do is this:
We need to add documentation to hook_install about the dangers of doing programatic configuration updates and not checking this.
Comment #18
alexpottThe solution in #11 will not work because if the post update exists at install time it will never run - exactly the same as hook_update_N
Comment #19
jibranYes, you are correct. Thank you @alexpott for explaining it.
It means there is no solution for #5?
Comment #20
samuel.mortensonWe could have a one-time update (normal hook_update_N) that checks if an icon is set and if not sets it, in addition to this change. That would cover legacy users without icons as well as people that run into this issue.
Comment #21
jibranI think we don't have to worry about #5 anymore. I thought about it and here is how #5 is not a problem.
- During development you enable the module.
- The config is updated on install.
- You export the config.
- You commit your changes and deploy your code on staging site.
- During the deploy you import all the config.
- During config import
file_browser
is installed.- And in the imported config the icon is already set.
- During module installation
file_browser_install
is called.- We copy the file again generate the new uuid but config has the old uuid.
-
!\Drupal::isConfigSyncing()
stops the config update.- Because the config has the old uuid that is why staging site was not showing the icon.
- We have to change the uuid either in DB or in config to solve the problem.
As @alexpott said all the update hook and post update hook are not run at the module installation and the DB scheme of the module is set to the latest N present in install file.
Comment #22
alexpott@jibran - #5 is irrelevant and misleading - if the module is being installed during a config import then it must have been installed on a site somewhere not as part of a part config import. So the config change will have occurred. If this is not the case then you have hacked your config and everything is on your head at that point.
Comment #23
alexpottHowever, it might be worth considering creating the file here with a fixed UUID. That way icon won't go missing after a config import.
Comment #24
alexpottThe other way of fixing this is way more complicated and I'm not sure will work very well. Basically only creating the file if not installing and then adding an event subscriber to the missing content entity event in the config importer and creating the file as necessary. But you're never going to know that you are creating the right file.
So fixing the UUID seems like a good compromise.
Comment #25
jibranI agree and totally understand this now but when the module is installed during config import then we'll get a new uuid. Isn't this right according to the patch in #19? And the config will have the old uuid which was generated at the time of first install.
Comment #26
alexpottRe #25 see #23 and #24 :)
Comment #27
jibranThank you for #24 it is much clear now.
Comment #28
jibran@alexpott how about this? I don't know if changing the uuid is like this is a good practice.
Comment #29
jibranComment #30
heddn#28 fixed the problem for me.
Comment #31
alexpottJust set the UUID of the file to a known value when you create the file. Imo this is okay. It is the same file. It's simpler with way less logic and messing about.
Comment #32
jibranThanks @heddn for testing it.
@alexpott I just wanted to check my approach with you. You are absolutely right about simplicity. Here is the updated patch and thank you for all your help.
Comment #33
jibranCreated #2712671: Fix the module install during config import for content browser.
Comment #35
samuel.mortensonLooks good in my manual testing, thanks @jibran et. all!
Comment #36
heddn#32 definitely doesn't solve the problem. Re-open or open a new issue?
Somewhere in
ConfigInstaller->installDefaultConfig()
, the default config isn't installed.!$this->isSyncing()
on ~ line 101 seems to part of the culprit.Comment #37
samuel.mortenson@heddn We should re-open this issue since this was committed so recently, do you have steps to reproduce so that I can replicate what you're seeing locally?
Comment #38
heddnFor now, so I can keep moving, here's a patch to revert back to #28. I don't think it is the solution.
It is simple to reproduce. Try install using config_installer profile and already have config setup for file_browser. By the time the hook_install is run, the default config isn't imported so it throws a PDO exception because the retrieval of the UUID returns a NULL.
Comment #39
heddnComment #40
heddnComment #41
heddnComment #42
alexpott@heddn it needed an upgrade path to set the uuid of the file and config to the default value and then everything should work.
Also tests.
Comment #43
heddnre:42, an update, as in a post install update?
Comment #44
alexpottYes
Comment #45
heddnHere we go. Let's go with hard-coding the UUID. The config isn't always available during the installation. To test this, I had to install the site using config_installer profile with an already provided config for the UUID.
Comment #46
alexpott@heddn actually we could use the ExtensionInstallStorage to read the default config...
Comment #47
jibranHere you go post update function.
Comment #48
heddnWorking on a combined patch to include #47, #46 & #45
Comment #49
heddnInterdiff is of the changes introduced to respond to #46.
Comment #50
jibran@alexpott do we really need that?
Comment #51
heddn@jibran, you do need it if you are using config_installer install profile. If you don't have it, the site installation fail 100% of the time.
Comment #52
alexpottThis would only work if you have it in sync storage. I Was trying to say to use ExtensionInstallStorage - with which you can read the default config provided by the module. That is the only thing that is guaranteed to be there.
Comment #53
alexpottHow about loading by the current UUID? Should be able to lose this complexity.
Comment #54
jibranSomething like this?
Comment #55
alexpottYou can't use this service - it is reading the schema. You just need to create a new ExtensionInstallStorage... something like:
new \Drupal\Core\Config\ExtensionInstallStorage(\Drupal::service('config.storage'));
Comment #56
jibranSorry, I don't know a lot about
ExtensionInstallStorage
. Please feel free to update the patch.Comment #57
heddnComment #58
jibranFYI I updated this function in #54. Sorry for not creating interdiff.
Comment #59
pguillard CreditAttribution: pguillard commented#57 fixed the missing uuid issue for me.
I do not master the subject, so I wonder if I should set it to RTBC...
Comment #60
samuel.mortensonI'm going to attempt to write tests for this before committing.
Comment #61
samuel.mortensonIdentical patch to #57 with tests this time. If the tests work as expected I'll commit.
Comment #62
samuel.mortensonComment #63
samuel.mortensonComment #68
samuel.mortensonCommitted! Thanks all for the iteration and review. Also, we finally have a test! I'll also be adding Javascript testing as a part of the Media sprints in July.