Fix the module install during config import

Thank you jibran, it works.

Would 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.

Ok, 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?

+++ b/file_browser.post_update.php
@@ -0,0 +1,21 @@
+function file_browser_post_update_set_file_browser_icon() {

This 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.

#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.

@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.

The current patch is not right... all you need to do is this:

 /**
 * Implements hook_install().
 */
function file_browser_install() {
  // Add an icon for File Browser.
  $data = file_get_contents(dirname(__FILE__) . '/file_browser_icon.png');
  $file = file_save_data($data, 'public://file_browser_icon.png', FILE_EXISTS_REPLACE);

  if (!\Drupal::isConfigSyncing()) {
    // Update our configuration to use the icon.
    $uuid = $file->uuid();
    $configuration = \Drupal::configFactory()->getEditable('embed.button.file_browser');
    $configuration->set('icon_uuid', $uuid);
    $configuration->save();
  }
}

We need to add documentation to hook_install about the dangers of doing programatic configuration updates and not checking this.

The 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

We 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.

@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.

However, it might be worth considering creating the file here with a fixed UUID. That way icon won't go missing after a config import.

The 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.

Re #25 see #23 and #24 :)

#28 fixed the problem for me.

Just 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.

Looks good in my manual testing, thanks @jibran et. all!

#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.

@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?

For 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.

@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.

re:42, an update, as in a post install update?

Yes

Here 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.

@heddn actually we could use the ExtensionInstallStorage to read the default config...

Working on a combined patch to include #47, #46 & #45

Interdiff is of the changes introduced to respond to #46.

@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.

+++ b/file_browser.install
@@ -10,13 +10,12 @@
+  $uuid = \Drupal::service('config.storage.sync')->read('embed.button.file_browser')['icon_uuid'];

This 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.

+++ b/file_browser.post_update.php
@@ -0,0 +1,37 @@
+  $files = \Drupal::entityTypeManager()
+    ->getStorage('file')
+    ->loadByProperties(['uri' => 'public://file_browser_icon.png']);
+  if (!empty($files)) {

How about loading by the current UUID? Should be able to lose this complexity.

+++ b/file_browser.install
@@ -12,8 +12,10 @@ function file_browser_install() {
+  if ($data = \Drupal::service('config.storage.schema')->read('embed.button.file_browser')) {

You 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'));

#57 fixed the missing uuid issue for me.

I do not master the subject, so I wonder if I should set it to RTBC...

I'm going to attempt to write tests for this before committing.

Identical patch to #57 with tests this time. If the tests work as expected I'll commit.

Committed! 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.