Problem/Motivation

I first noticed this at #3039026-8: Deprecate file_directory_temp() and move to FileSystem service

The docs in core/lib/Drupal/Core/Extension/module.api.php are seriously weird for hook_install() and hook_uninstall()

function hook_install() {
  // Create the styles directory and ensure it's writable.
  $directory = file_default_scheme() . '://styles';
  \Drupal::service('file_system')->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
}

...

function hook_uninstall() {
  // Remove the styles directory and generated images.
  \Drupal::service('file_system')->deleteRecursive(file_default_scheme() . '://styles');
}

A) It's too bad all these examples are using file-related methods that are in the process of being deprecated and killed. ;)

B) What if the site admin changes their default file scheme between when this hypothetical module was installed and uninstalled? :/ Maybe on uninstall we recursively delete an entirely wrong directory(!). Ugh.

Proposed resolution

I don't really care what we replace this with, but I think we need to remove these confusing and frankly dangerous examples.

Remaining tasks

  1. Come up with more reasonable examples for hook_install() and hook_uninstall().
  2. Write the patch.
  3. Reviews/nitpicks/bikeshed
  4. RTBC
  5. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

pandaski’s picture

Status: Active » Needs review
FileSize
1.21 KB

A patch for this.

hook_install - Set the site email address to something that is not sendmail_from

hook_uninstall - Delete remaining general module variables.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tvb’s picture

Patch 3039296-2.patch could not be applied to Drupal core 8.9.x-dev.

The patch is outdated. The hook functions in 8.9.x-dev have a new parameter $is_syncing.

Rerolled and attached a new patch.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

LGTM!

dww’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for moving this forward. I completely forgot about it. ;)

1 tiny-winy nit:

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -316,8 +317,8 @@ function hook_modules_uninstalled($modules, $is_syncing) {
+  \Drupal::state()->delete('node.node_access_needs_rebuild');

I don't think we should be recommending to delete another module's state. The example right below uses 'mymodule.service'. Let's use a 'mymodule.x' key to delete from state, too.

Thanks,
-Derek

abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
639 bytes

Hi @dww
Is this new patch dose what you suggested on #6 comment.

dww’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thanks. That works. ;)

jungle’s picture

-use Drupal\Core\File\FileSystemInterface;

See https://www.drupal.org/pift-ci-job/1643161

Keep it as RTBC, as this is a tiny change.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -241,9 +240,10 @@ function hook_modules_installed($modules, $is_syncing) {
+  // Set the site email address to something that is not sendmail_from.
+  \Drupal::configFactory()->getEditable('system.site')
+    ->set('mail', 'example@example.com')
+    ->save(TRUE);
   if (!$is_syncing) {
     // Modify a configuration value because we're not syncing.
     \Drupal::configFactory()->getEditable('system.file')->set('default_scheme', 'private')->save();

This isn't a good example because config changes should only be made when not config syncing... see the code below...

jungle’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
984 bytes

@alexpott, thanks for reviewing! Adjusted.

dww’s picture

Status: Needs review » Needs work

Ugh, hate to do this, but NW again. With the patch applied, the full example is now:

function hook_install($is_syncing) {
  if (!$is_syncing) {
    // Set the site email address to something that is not sendmail_from.
    \Drupal::configFactory()->getEditable('system.site')
      ->set('mail', 'example@example.com')
      ->save(TRUE);
    // Modify a configuration value because we're not syncing.
    \Drupal::configFactory()->getEditable('system.file')->set('default_scheme', 'private')->save();
  }
}

So the hunk we're adding is entirely duplicate with the example already in place.

  1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -241,10 +240,11 @@ function hook_modules_installed($modules, $is_syncing) {
    -  // Create the styles directory and ensure it's writable.
    -  $directory = \Drupal::config('system.file')->get('default_scheme') . '://styles';
    -  \Drupal::service('file_system')->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
       if (!$is_syncing) {
    +    // Set the site email address to something that is not sendmail_from.
    +    \Drupal::configFactory()->getEditable('system.site')
    +      ->set('mail', 'example@example.com')
    +      ->save(TRUE);
    

    We should either just remove the old thing entirely and leave the existing example under !$is_syncing, or we should replace with something not related to config that makes sense outside of the !$is_syncing check. TBD.

  2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -316,8 +316,8 @@ function hook_modules_uninstalled($modules, $is_syncing) {
    +  // Delete remaining general module variables.
    +  \Drupal::state()->delete('mymodule.lousy_module_compatibility');
    

    Comment mentions "module variables" but the code is deleting something from state. I think most people would consider "module variables" config settings, not state. Let's make sure the comment matches the code here and say something like "Remove custom module variables from the persistent state service." or some such...

jungle’s picture

  1. +++ b/tests/src/Kernel/SchemaTest.php
    @@ -0,0 +1,31 @@
    +    $this->config('conflict.settings')->set('resolution_type.default.default', 'dialog')->save();
    +    $this->config('conflict.settings')->set('resolution_type.node.default', 'dialog')->save();
    +    $this->config('conflict.settings')->set('resolution_type.node.article', 'dialog')->save();

    The above is from my review of #3122375: Add conflict.schema.yml comment #16, 12 hours ago -- call save() 3 times, which can be avoided. So my suggestion to #12.1: let's show an example of chaining only. For instance:

    \Drupal::configFactory()->getEditable('system.site')
      // Set the site email address to something that is not sendmail_from.
      ->set('mail', 'example@example.com')
      // The set() call can be chained.
      ->set('name', 'Another Drupal site')
      ->save();
  2. #12.2 nice catch!
  3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -241,10 +240,11 @@ function hook_modules_installed($modules, $is_syncing) {
    +    // Set the site email address to something that is not sendmail_from.
    

    Needs changing sendmail_from. or just Set the site email address.

@dww, thank you!

jungle’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
1.49 KB
  1. Replaced with chained calls in hook_install()
  2. Rewrote hook_uninstall() completely. removeContent() does not have to be wrappered inside if(!$is_syncing), so replaced with removeConfig()
    function hook_uninstall($is_syncing) {
      // Remove module-specific state setting.
      \Drupal::state()->delete('mymodule.foo_state');
      if (!$is_syncing) {
        // Remove module-specific config which can not be removed during
        // uninstallation automatically.
        \Drupal::service('mymodule.foo_service')->removeConfig();
      }
    }
jungle’s picture

Two questions out of scope here

  1. Should we add parameter $is_syncing to all hook related implementations, such as node_uninstall(), even it's not being used?
  2. function image_uninstall() {
      // Remove the styles directory and generated images.
      /** @var \Drupal\Core\File\FileSystemInterface $file_system */
      $file_system = \Drupal::service('file_system');
      try {
        $file_system->deleteRecursive(\Drupal::config('system.file')->get('default_scheme') . '://styles');
      }
      catch (FileException $e) {
        // Ignore failed deletes.
      }
    }
    

    Should the above be the blow

    function image_uninstall($is_syncing) {
      if (!$is_syncing) {
        // Remove the styles directory and generated images.
        /** @var \Drupal\Core\File\FileSystemInterface $file_system */
        $file_system = \Drupal::service('file_system');
        try {
          $file_system->deleteRecursive(\Drupal::config('system.file')->get('default_scheme') . '://styles');
        }
        catch (FileException $e) {
          // Ignore failed deletes.
        }
      }
    }
jungle’s picture

Title: Fix hook_uninstall() example in core/lib/Drupal/Core/Extension/module.api.php » Fix examples in hook_install() and hook_uninstall()

Changing the title to include hook_install().

dww’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -241,12 +240,12 @@ function hook_modules_installed($modules, $is_syncing) {
    -    // Modify a configuration value because we're not syncing.
    

    We should keep this, but make it plural:

    "Modify configuration values because..."

  2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -241,12 +240,12 @@ function hook_modules_installed($modules, $is_syncing) {
    +    // Alter the site name and the site-wide email address.
    

    Maybe we don't need this if we keep the above? Hopefully this much is self-documenting, but I guess as API examples, it doesn't hurt to be a bit more verbose here. I'm happy either way. Not sure what core committers will prefer. /shrug

Otherwise, looks good to me.

Thanks!
-Derek

jungle’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
664 bytes

Bringing back #17.1, Modify..., and removed #17.2 Alter the site name ... as it looks redundant right after Modify... and the code is self-documented.

Again thank you for reviewing, @dww!

andrey.troeglazov’s picture

Status: Needs review » Reviewed & tested by the community

I have checked the patch from #18 and it looks good for me.
Also it is applied without any issues.
Thank you.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -241,12 +240,12 @@ function hook_modules_installed($modules, $is_syncing) {
    -    // Modify a configuration value because we're not syncing.
    -    \Drupal::configFactory()->getEditable('system.file')->set('default_scheme', 'private')->save();
    +    // Modify configuration values because we're not syncing.
    +    \Drupal::configFactory()->getEditable('system.site')
    +      ->set('name', 'Yet another Drupal site')
    +      ->set('mail', 'example@example.com')
    +      ->save();
    

    So I'm really not convinced this is a good example. A module really doesn't have any business setting these particular config values. I don't think it is this codes job to show people chained calls. The example that was here at least has some practical value in a module that's enforcing security settings.

  2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
    @@ -316,10 +315,12 @@ function hook_modules_uninstalled($modules, $is_syncing) {
    -    \Drupal::service('mymodule.service')->removeContent();
    +    // Remove module-specific config which can not be removed during
    +    // uninstallation automatically.
    +    \Drupal::service('mymodule.foo_service')->removeConfig();
    

    This change should never ever be the case. If it can't be removed automatically during uninstall that the module can not be installed / uninstalled via config sync because config dependencies are obviously wrong. I'm not saying that the original fake code is better but this is worse.

jungle’s picture

Extracted 2 real implementations from the core.

  1. /**
     * Implements hook_uninstall().
     */
    function shortcut_uninstall() {
      // Theme settings are not configuration entities and cannot depend on modules
      // so to unset a module-specific setting, we need to unset it with logic.
      foreach (['seven', 'claro'] as $theme) {
        if (\Drupal::service('theme_handler')->themeExists($theme)) {
          \Drupal::configFactory()
            ->getEditable("$theme.settings")
            ->clear('third_party_settings.shortcut')
            ->save(TRUE);
        }
      }
    }
    

    shortcut_uninstall() should match the use case of #20.2, third party settings can not be removed automatically during uninstalling.

  2. /**
     * Implements hook_uninstall().
     */
    function node_uninstall() {
      // Delete remaining general module variables.
      \Drupal::state()->delete('node.node_access_needs_rebuild');
    }
    

    Maybe it‘s enough to add simple examples just like node_uninstall(); For instance:

    function hook_install() {
      // Set general module variable.
      \Drupal::state()->set('mymodule.foo', 'bar');
    }
    
    function hook_uninstall() {
      // Delete remaining general module variables.
      \Drupal::state()->delete('mymodule.foo');
    }
    
  3. Or to abstract from existed real implementations.

No patch, but set back to needs review to receive suggestions of what to do next.

jungle’s picture

Status: Needs work » Needs review
alexpott’s picture

theme settings are incredibly special and awkward. As a blanket statement

third party settings can not be removed automatically during uninstalling.

is not true. \Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval() specifically removes third party settings on config entities where the providing module is being uninstalled. Unfortunately theme settings are not config entities. And whilst shortcut_uninstall() probably should have an a check on $is_syncing and only do that when not syncing there's probably no harm from the current implementation.

Doing \Drupal::state() management in these hooks is a good example.

jungle’s picture

FileSize
1.66 KB

@alexpott, thank you! Learned.

Using Doing \Drupal::state() management as examples.

init90’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for work here! Updated examples are simple and correct, so I think it's ready.

  • catch committed 0bcbf2f on 9.1.x
    Issue #3039296 by jungle, abhisekmazumdar, pandaski, tvb, dww, alexpott...

  • catch committed bda5391 on 9.0.x
    Issue #3039296 by jungle, abhisekmazumdar, pandaski, tvb, dww, alexpott...

  • catch committed c929177 on 8.9.x
    Issue #3039296 by jungle, abhisekmazumdar, pandaski, tvb, dww, alexpott...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.