Using "New comments" field in a view breaks if history module not enabled

Here's a simple fix

Otoh it makes sense to disable the plugin when no history modules enabled

And here's a test

Thank you, looks great

Looks good to me, and should go in during RC if possible, tagging for triage.

I think the Drupal\comment\Plugin\views\field\NodeNewComments plugin also needs to be adding a dependency on the history module so that a view which depends on it is listed when uninstalling the history module. Doing this will break the new test :)

Discussed with catch and we agreed that this would be a nice bug to fix asap.

Also @xjm is plus one too.

Found the very similar issue that went in recently: #2591147: Filter criteria for views, option "Content is updated" - causes a SQL syntax error

Sorry, my point in #12 was that #2591147: Filter criteria for views, option "Content is updated" - causes a SQL syntax error and this issue are fixing the same bug in different places, so it makes me wonder whether there is a fix that will fix both rather than special-casing comment all over the place in history views plugins.

tested manually imported view, it's removed after uninstall but not in test...

Fixing the test - you need to rebuild the router after uninstall - that would happen if part of a regular request.

Thanx a lot!

+1

re #13 the fix in #2591147: Filter criteria for views, option "Content is updated" - causes a SQL syntax error is not really removing the comment dependency - it's just burying it behind some tricky to follow logic. The fix here makes the dependency on the history module of the NodeNewComments plugin explicit and therefore, I think this is the correct fix.

Quite how a user might discover that they get this additional functionality by installing the history module is another question that I have no idea how to answer.

Unfortunately we need an upgrade path because this is adding dependencies to views.

The upgrade path can use the post update functionality and therefore use the Views and config entity APIs to fix all of this.

Updating patch for 8.1.x and 8.2.x. hence interdiff isn't attached.

Directly updating 8.2.x patch first.

Fixing typo for 8.2.x patch.

Fixing errors for patch in 8.1.x branch.
Interdiff taken from #27 patch.

This bug is still present, I've updated the steps to reproduce issue to be more precise to triage the issue.

The patch applies, the patch resolves this error by making the field not available unless the history module is enabled.

updating remaining tasks

I'm not sure if the "missing config dependencies" tag should be kept so that we know config dep changed, or if that tag means there is a todo to add missing config dep. so leaving the tag.

Update path is next step here

Working on this as part of DrupalCon Nashville!

Issue still happening within the 8.6.x branch. Fixed issue with upgrade path using the suggestion of using hook_post_update_NAME.

Needed a reroll, so did that.

Changed the update hook a bit, not convinced that turning on the history module and not removing the field is the right way to go here, but it is the least destructive, so let's go with that for now.

Added an upgrade path test, it doesn't check for the history module getting switched on because we don't have a dump where it's turned off and uninstalling it in the test does unexpected stuff to the view.
So just this for now.

Updated the tags a bit.

+++ b/core/modules/comment/comment.post_update.php
@@ -14,3 +17,28 @@ function comment_removed_post_updates() {
+  if ($enable_history_module) {
+    \Drupal::service('module_installer')->install(['history']);

if we get this far, do we need to keep the batch running?

I.e should we set $sandbox['#finished'] = 1

edit: Yes, because we need the view's dependencies to be updated too

This is looking good to me, I'm not sure if we need an update path test to ensure the module is enabled, we already have the dependency test which is implicitly testing the update path.

We do however need to introduce the deprecation error this is introducing in the test