Enable Announcements Feed by default, but allow opt-out

Thought about adding a link to the (currently draft) CR to the hook_update_N() comment to clearly signpost instructions for the opt-out - e.g.:

/**
 * Enable the Announcements Feed module by default - see https://www.drupal.org/node/3424912 to opt out.
 */
function system_update_7087() {
  // Allow opt-out.
  if (!variable_get('announcements_feed_enable_by_default_opt_out', FALSE)) {
    module_enable(array('announcements_feed'), FALSE);
  }
}

However this ends up being output like this by drush:

$ drush updb
 System       7087  Enable the Announcements Feed module by default - see https:www.drupal.orgnode3424912 to opt out.

..so that doesn't quite work.

I think maybe just keep it really simple.

I've pushed a very basic update hook with a comment that looks like this via drush:

$ drush updb

 System       7087  Enable the Announcements Feed module - see Release Notes for opt-out details.

I'd suggest that we don't add the opt-out variable to default.settings.php as this is a one-off situation; once the update hook has been run, the variable won't do anything.

So if this looks like it'll do the job, we just need to write up the CR and snippet for the Release Notes.

@Fabianx approved this in a different channel. However I won't mark it as RTBC just yet...

We need to commit the actual module first, and will then perhaps need to start a new MR.

@poker10 has also pointed out that we need to consider whether this is enabled by default for a clean install. I think it probably should be.

I think it makes sense to enable announcements_feed by default in the standard profile, but not minimal.

I've pushed a commit for that to the MR.

As this is done via an entry in the .info file of the profile, the opt-out variable won't do anything there... I think that's okay though. It's really for existing sites to opt-out of the module being enabled by the update hook.

@Fabianx suggested that the opt-out should also work for a fresh install, which seems reasonable but means we'll need a slightly different approach.

I've removed the entry from standard.info and put the same code that checks the opt-out in the system module's update hook into standard.install.

For a quick manual test, here are the relevant watchdog messages after running a drush site-install:

 38  05/Mar 08:25  actions   notice    Action 'Unpublish comment' added.              
 37  05/Mar 08:25  actions   notice    Action 'Publish comment' added.                
 36  05/Mar 08:25  system    info      standard module enabled.                       
 35  05/Mar 08:25  system    info      standard module installed.                     
 34  05/Mar 08:25  system    info      announcements_feed module enabled.             
 33  05/Mar 08:25  system    info      announcements_feed module installed.           
 32  05/Mar 08:25  system    info      toolbar module enabled.                        
 31  05/Mar 08:25  system    info      toolbar module installed.

The module seems to be enabled just before the list of dependencies in the .info file, which should be fine in this case.

I've also done a manual test to confirm that adding this to settings.php

$conf['announcements_feed_enable_by_default_opt_out'] = TRUE;

...means that the module is not enabled during installation.

I think it's preferable to do this in the standard install profile so that we don't install the module as part of the minimal profile, as that doesn't seem like it'd be appropriate.

Thanks for working on this @mcdruid!

The tests are failing, because we missed a version property in the announcements module info file and Drupal takes the core version from the first enabled module. Therefore the update module thinks that the core is out of date and sends an email about the update (which causes an error later, because sendmail is not available).

We need to fix this first: https://www.drupal.org/project/drupal/issues/3425698 🐛 Announcements module info file is missing the version property Needs review

Then tests would be hopefully green. Otherwise, I think the change here looks good (and also the placement in the standard_install() function). If we can add a simple test for the opt-out variable (maybe just check if there is/is not a link in the toolbar after install), it would be great.

Per @poker10 the problem is that ModuleUnitTest::testModuleList() compares the list of modules from .info files with what module_list() reports has actually been enabled.

With any luck, the commit I just pushed should make the test pass, as we set the opt-out flag when the standard profile is being installed inside a test.

As an added bonus, this also tests the opt-out itself (at least in the install profile).

I would like to find a cleaner way of doing this though; I tried adding a \ModuleUnitTest::setUp() in which to set the opt-out variable, but that doesn't work because the parent setUp() method wipes $conf clean before installing the test site.

We could mess about more with setting a global there, and checking for it during the install in \DrupalWebTestCase::setUp but I'm not sure that'd be any better than adding the check in standard_install()?

Tests pass with that hack.

Manually testing the opt-out flag in settings.php

without:

$ ddev drush -y si && ddev drush pml | grep -i announcement
...snip...
Installation complete.  User name: the-head-honcho  User password: ...snip...

 Core            Announcements (announcements_feed)                                Module  Enabled        7.100-dev

with the opt-out:

$ ddev drush -y si && ddev drush pml | grep -i announcement
...snip...
Installation complete.  User name: the-head-honcho  User password:  ...snip...

 Core            Announcements (announcements_feed)                                Module  Not installed  7.100-dev

I've also manually tested system_update_7087() with and without the opt-out variable set.

So the only remaining question for me is whether we can think of a cleaner way to achieve something similar to the check for the drupal_test_info global in standard_install()?

We also need to write the CR and Release notes snippet.

It seems like that there are multiple places in core / contrib (like token module, views module), where practically the same check is done (with $GLOBALS['drupal_test_info']), so from this point of view it is not that hacky solution. See for example: https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/common.inc...

It is only hacky in a way, that this way we will never automatically install the Announcements module in tests directly with standard profile, so it will not simulate the exactly same flow as a user installing D7 will have. But we are installing the module in various tests in announce_feed_test.test, so I do not this this is a big issue.

A small nitpick, the module name is Announcements, so we could probably update this (can be done also on commit):

+ * Enable the Announcements Feed module - see Release Notes for opt-out details.

All tests are green and the rest of the MR looks good to me.

I have drafted a text of the CR, feel free to update it - https://www.drupal.org/node/3424912 →

I tweaked the module name in the update hook per #13.

I've also changed it so that standard_install() only switches on the opt-out flag for the specific test class that otherwise fails. This meant adding a new element to $GLOBALS['drupal_test_info'] which feels a bit icky, but I think it's a reasonable compromise.

This means we're getting all tests to pass, plus testing the opt-out itself (within the profile if not the update hook) but all the other tests that use the standard profile will get the module enabled by default. That's good because it means we're more likely to catch any regressions involving the new module.

With the module enabled by default in the standard profile, we could probably tweak some of the tests in announce_feed_test.test where it shouldn't be necessary any more to explicitly enable the new module, but I don't think we need to do that immediately (and it shouldn't do any harm if we never make that change).

Erm, incidentally, should announce_feed_test.test be just announce_feed.test? Again, not an urgent thing to address.

Good idea to disable the auto-install only for the one specific test class! I think that this is the best compromise we can achieve in combination with a way how we want to enable this and also allow opt-out.

I have also tested the MR manually:

On existing Drupal installation - running update.php with the settings configuration option, the module was not installed. Without the settings configuration option, the module was installed and enabled and was working as expected (together with the new menu link added to the toolbar).

On new Drupal 7 installation (without settings.php) - you need to add the configuration variable to the default.settings.php, as the main settings.php is overwritten multiple times (as it is copied from default.settings.php). We will document this in CR.

All tests are still green. So I think this is good to go. Committed and pushed this. Thanks!

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