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.

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.

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.