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
- Come up with more reasonable examples for hook_install() and hook_uninstall().
- Write the patch.
- Reviews/nitpicks/bikeshed
- RTBC
- Commit
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#24 | 3039296-24.patch | 1.66 KB | jungle |
Comments
Comment #2
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedA patch for this.
hook_install - Set the site email address to something that is not sendmail_from
hook_uninstall - Delete remaining general module variables.
Comment #4
tvb CreditAttribution: tvb commentedPatch 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.
Comment #5
joachim CreditAttribution: joachim commentedLGTM!
Comment #6
dwwThanks for moving this forward. I completely forgot about it. ;)
1 tiny-winy nit:
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
Comment #7
abhisekmazumdarHi @dww
Is this new patch dose what you suggested on #6 comment.
Comment #8
dwwCool, thanks. That works. ;)
Comment #9
jungleSee https://www.drupal.org/pift-ci-job/1643161
Keep it as RTBC, as this is a tiny change.
Comment #10
alexpottThis isn't a good example because config changes should only be made when not config syncing... see the code below...
Comment #11
jungle@alexpott, thanks for reviewing! Adjusted.
Comment #12
dwwUgh, hate to do this, but NW again. With the patch applied, the full example is now:
So the hunk we're adding is entirely duplicate with the example already in place.
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.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...
Comment #13
jungleThe 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:
Needs changing
sendmail_from
. or justSet the site email address.
@dww, thank you!
Comment #14
junglehook_install()
hook_uninstall()
completely.removeContent()
does not have to be wrappered insideif(!$is_syncing)
, so replaced withremoveConfig()
Comment #15
jungleTwo questions out of scope here
$is_syncing
to all hook related implementations, such asnode_uninstall()
, even it's not being used?Should the above be the blow
Comment #16
jungleChanging the title to include hook_install().
Comment #17
dwwWe should keep this, but make it plural:
"Modify configuration values because..."
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
Comment #18
jungleBringing back #17.1,
Modify...
, and removed #17.2Alter the site name ...
as it looks redundant right afterModify...
and the code is self-documented.Again thank you for reviewing, @dww!
Comment #19
andrey.troeglazov CreditAttribution: andrey.troeglazov at DrupalJedi commentedI have checked the patch from #18 and it looks good for me.
Also it is applied without any issues.
Thank you.
Comment #20
alexpottSo 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.
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.
Comment #21
jungleExtracted 2 real implementations from the core.
shortcut_uninstall()
should match the use case of #20.2, third party settings can not be removed automatically during uninstalling.Maybe it‘s enough to add simple examples just like node_uninstall(); For instance:
No patch, but set back to needs review to receive suggestions of what to do next.
Comment #22
jungleComment #23
alexpotttheme settings are incredibly special and awkward. As a blanket statement
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.
Comment #24
jungle@alexpott, thank you! Learned.
Using Doing \Drupal::state() management as examples.
Comment #25
init90Thanks for work here! Updated examples are simple and correct, so I think it's ready.
Comment #29
catchCommitted/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!