Statistics module installs {node_counter} even if Node is uninstalled

Looks like the module has no sense without the node module.
So let just add it into dependencies.
What do you think?

I agree with @dmitrii, anyone oppose?

I'd like to make it entity counter one day, but as long as it's node counter it should depend on node module.

Think it's time to get this committed.

I think this makes sense -- as far as I know, the Statistics module does not work without the Node module. The statistics module's blog plugin straight up calls Node module functions without checking for the module's existence, for example. The missing dependency is probably a legacy of Node being required in D7 and earlier. We could support entities of other types in the future, but I agree that that is not in scope during the D8 beta and better to just go for the simpler fix now.

Looks like we also have the subsystem maintainer's signoff with @timmillwood's RTBC above.

We should create a small change record for this since an existing site/profile could have Statistics enabled without Node currently.

I also considered that we might need an upgrade path for this now that we are requiring hook_update_N() in core. @alexpott suggested that the update function should enable Node if the Statistics module was enabled without it; however, I'm not sure that's a good idea since the Statistics module without Node basically just doesn't do anything, but Node does a lot of stuff.

Let's test what happens when this patch is applied to an existing site that has Statistics but not Node to figure out what we might need for an upgrade path (or not) and ensure that we don't end up in an unrecoverable state. Things to test would be placing the statistics block, uninstalling the Statistics module, installing or uninstalling the Node module, etc.

This can be reverted when #2532334: Count all content entities views in the Statistics module is complete, but planning for this in 8.1.x or 8.2.x.

I have tested a site with statistics module enabled but node module disabled and nothing breaks. The statistics block can be placed, but doesn't do anything, the statistics module can be disabled and doesn't freak out because node isn't there etc.

Writing a patch now to enable node on update.

Added update hook

I have no idea how to use statistics module in current state without node module. What do you think about to uninstall the module in statistics_update_8001?

@dmitrii - The main issue I have with that is uninstalling the statistics module in statistics_update_8001 will cause any data to be lost. I understand this is all very edge case, and if the node module is not installed then statistics module should have no data anyway, but I'd rather not risk any data loss at all.

What we should do at least is to show a message that the node module was enabled. Sites might not expect that to happen, so better warn them.

@timmillwood
What do you think about the following behaviour?
if node_counter table is empty && no other modules depending on the statistics module - unistall the statistics && dsm to the user
else enable the node module && dsm to the user

@dmitrii - Sounds like a plan!

Still not sure about this, but adding patch anyway.

If node is disabled and node_counter is empty then statistics will be uninstalled
if node is disabled and node_counter is note empty then node will be installed. However I'm starting to think what's the point in enabling node, because all the data in node_counter will be irrelevant because there will be no nodes or even content types. So might be leaning towards #7.

To be sure looks like we should also check if there are any modules depending on the statistics module. (ModuleHandler::buildModuleDependencies).
But it becomes too complex... Maybe we should just return back to #1 because of #5?

I think this needs to be bumped to major before it becomes more of an issue on the upgrade path. Dependencies can get scary if not managed properly.

Also assigning to @xjm, I think #1, #6, or #12 are all valid options, but agree with #9 that whatever we do it needs a dsm.

So here's my choices:
- Show dsm if node is not installed, but don't install/uninstall anything
- Install node if it's not already installed and show dsm
- Uninstall statistics if node is uninstalled (blowing away useless data) and show dsm
- Install / uninstall modules depending on if there is statistics data (although nids in statistics table will be useless as nodes would've been deleted when node was uninstalled)

Note: This all becomes irrelevant when I complete #2532334: Count all content entities views in the Statistics module, although I plan to do that for 8.1.x

As it's been a week since my last comment, and we really need this in before RC1, I thought I'd make an executive decision. The statistics module is useless without the node module and if the node module used to be enabled but then was uninstalled any data in the statistics module would be useless too so should be purged, therefore uninstalling is the best (and only true) option.

Here's a patch to set node as a dependency for statistics and uninstall statistics if node is not enabled. A dsm The statistics module depends on the node module and has therefore been uninstalled. has also been added.

Enjoy.

I think the patch in #15 is the easiest thing to do at this point. However, we need an update path test to check if the module was actually uninstalled and its table was deleted.

Just found a few code style nitpicks:

+++ b/core/modules/statistics/statistics.install
@@ -57,3 +57,13 @@ function statistics_schema() {
+      \Drupal::service('module_installer')->uninstall(array('statistics'), TRUE);
+      drupal_set_message('The statistics module depends on the node module and has therefore been uninstalled.','status');

Wrong indentation here (two extra spaces). Also, missing space before 'status'.

Added test to make sure the the statistics module gets disabled after updates run.

The test looks good to me. Here's a small review for it:

1. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,55 @@
   + * Contains \Drupal\block\Tests\Update\BlockContextMappingUpdateTest.
   

   Let's update the class name here :)

2. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,55 @@
   +  protected static $modules = ['statistics'];
   

   I think it's better for readability to also include the node module here, even if the standard install already has it.

3. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,55 @@
   +  public function testUpdate() {
   

   Needs a small doc block, something like: "Tests the update path for the Statistics module."

4. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,55 @@
   +    // Make sure the node module was in face disabled.
   

   in face -> in fact?

5. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,55 @@
   \ No newline at end of file
   

   You know what to do about this :P

Taking @amateescu's comments onboard.

1. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,73 @@
   +    \Drupal::service('module_installer')->uninstall(array('node'), FALSE);
   

   Isn't node module already disabled over here because its set as dependency?

2. +++ b/core/modules/statistics/src/Tests/StatisticsUpdateTest.php
   @@ -0,0 +1,73 @@
   +    $this->assertFalse(\Drupal::moduleHandler()->moduleExists('node'), 'Node module is disabled');
   

   Even if it wouldn't, it would be good idea to test here :)

Node module is installed when the test is setup, and uninstalled as you mentioned in #24.1, but none of the dependencies of node are uninstalled. The assertFalse checks node has been disabled and this passes fine.

The two tests that fail are the final "Check the Schema version", which kinda should fail, because if statistics has been disabled by the update hook there shouldn't be a schema version? but just there to kinda debug. Then "Make sure the statistics module was disabled by the update hook" also fails, so it looks as though the update hook hasn't completed.

However locally I added some assertions to UpdatePathTestBase and found statistics is listed as a module that requires updates, and the update.php does complete. I was unable to assert the dsm, running it manually (without JS like testbot does) the dsm only flashes up for a few milliseconds, so that might be why.

+++ b/core/modules/statistics/statistics.install
@@ -57,3 +57,13 @@ function statistics_schema() {
+    drupal_set_message('The statistics module depends on the node module and has therefore been uninstalled.', 'status');

drupal_set_message() is the wrong thing to do. Return the message instead so drush theoretically can also work with it

Updated with suggestion from #26 and also removed (unsuccessful) automated tests, I think a manual test would be ok for such a small trivial patch.

I tested this manually using this scenario:

- installed d8 with the minimal profile
- uninstalled the node module
- enabled the statistics module (the 'node_counter' table showed up in the db)
- applied the patch from #27
- went to update.php which had one update as expected
- ran the update, the successful message was returned
- the statistics module was uninstalled (by looking at admin/modules)
- the 'node_counter' table was gone

Let's see if committers agree that this manual testing is enough for this patch :)

@timmillwood, we still need to add a change record though.

Draft change record added: https://www.drupal.org/node/2545980

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed c8907eb and pushed to 8.0.x. Thanks!