Move default views to their respective modules

[ghost]

As per #147 (comment), default views should be provided by their module and not Views itself.

---

PR by @BWPanda: backdrop/backdrop#2391
PR by @jenlampton backdrop/backdrop#2464

[ghost]

Tried to have a crack at this, but simply moving the taxonomy and comment view config files to their respective modules didn't seem to work... Do you have to register config files somewhere?

[quicksketch]

You shouldn't have to. Just putting the views.view.[view_name].json file in core/modules/taxonomy/config should do the job. Views registers all files with the views.view. prefix, and core should automatically install any files in a module's config directory upon install.

In order to get the "Revert" option working in the View UI, you'll also need to update the "module" key in the JSON config file to point at the original module.

[ghost]

I created a new site based on this code and nothing seems to have changed... If I disable AND uninstall the Comment module, then refresh the Views overview page, the Recent Comments default view is still present. However when I then try to enable it, I get a whole bunch of errors about Missing handlers. View still lets me edit it, but the fields for example say Broken/missing handler. Another error that appears when trying to add a new field (that's hopefully a little more helpful) is: Notice: Undefined index: base in view->get_base_tables() (line 752 of /var/www/backdrop/core/modules/views/includes/view.inc).

[quicksketch]

If I disable AND uninstall the Comment module, then refresh the Views overview page, the Recent Comments default view is still present.

Hmm... If the view hasn't been enabled and is in the default state, it seems like we should remove it doesn't it? We could add a check into hook_uninstall() for Taxonomy and Comment module to check if this is the case and clean up the installed views if they are in the default state. Modified Views should probably be left in place and left to the user to delete.

You can check by checking the "storage" property in the JSON file:

$config = config('views.view.comments_recent');
if ($config->get('storage') == VIEWS_STORAGE_DEFAULT) {
  $config->delete();
}

However when I then try to enable it, I get a whole bunch of errors about Missing handlers.

This is acceptable IMO. It's the same behavior you'd get with any view if you disabled a module after using its fields or filters anywhere. We actually have the same behavior in Layouts (the block shows up as "Broken" and is still there, but can be removed).

[ghost]

It's been a long time coming, but I just made a new PR for this issue (no, I haven't been working on it all these years 😝).

[klonos]

...I have reviewed the PR and it seems good code-wise, but when testing in the PR sandbox I have noticed that the "Recent comments" view was still available even when the Comments module was disabled and uninstalled.

I would expect that:

1. if a module that provides a view is disabled, then it should not be possible to enable the view.

2. if the module is also uninstalled, then the view provided by that module should not be listed in the Views UI at all.

[klonos]

...I forgot to mention that before disabling and uninstalling the module, I had also reverted the view.

[klonos]

...on second thought, point 2 in my comment above makes sense only in the case of contrib modules, AND specifically when the module files have been removed from the codebase. In case of core modules (which are always present in the codebase), I would expect that enabling a disabled view would ask you to also enable the module providing it. But I realise that we do not have any such logic in place, so perhaps a follow-up issue?

[ghost]

when testing in the PR sandbox I have noticed that the "Recent comments" view was still available even when the Comments module was disabled and uninstalled.

I noticed the same. I think it may have to do with the 'storage' config option not always being available...?

[ghost]

Based on this comment in view.inc:

Storage settings are only saved if this view was provided by a module.
Otherwise VIEWS_STORAGE_NORMAL is assumed on load.

I've gone ahead and changed the PR to delete views on uninstall if the view has not been overridden (as opposed to if it's in default state). This is because, as far as I can tell, unless a module-provided view has been overridden, the 'storage' option may not exist. We can therefore assume that it's in its default state, since we know it's a module-provided view (as opposed to a user-created one, which would have a 'normal' state).

In other words, I simply changed if ($config->get('storage') == VIEWS_STORAGE_DEFAULT) { to if ($config->get('storage') != VIEWS_STORAGE_OVERRIDE) { and the view is now properly deleted on module uninstall.

[klonos]

Not sure that this works as expected quite yet. I have tested this in the PR sandbox:

1. Visited the modules listing page and confirmed that the Comment module is enabled.
2. Visited the views listing page and confirmed that the "Recent comments" view was included. It was disabled.
3. I reverted the "Recent comments" view (to make sure that it is in its "default" state).
4. I disabled the Comment module.
5. The "Recent comments" view was still listed.
6. Tried to enable the "Recent comments" view -> it got enabled 👎 ...I expect views supplied by modules to not be possible to be enabled if the respective module is disabled - either disable their "Enable" button, or require the module to be enabled in order to enable the view (separate ticket perhaps)
7. I uninstalled the Comment module.
8. Visited the view listing page -> the "Recent comments" view was still listed, and in fact enabled 👎 ...it's OK for views supplied by modules to be listed even if the module is disabled or uninstalled (so long as the module code is present in the codebase - which is always true for core modules), but to have the view enabled while the respective module is disabled and uninstalled seems really odd.

[ghost]

I agree that we can do things better regarding disabled modules, etc., but I wonder what the scope of this issue is... If it's simply to move the views out of Views and into their respective modules, then maybe we should create a separate issue for @klonos' suggestions. If we want this issue to be inclusive of those suggestions though, I'll try looking at this later to see what can be done.

[klonos]

I am a fan of "baby steps" ...so we can have a separate issue for improving the handling of views provided by modules. That's why in my initial review comment a few days back I said:

I have reviewed the PR and it seems good code-wise ...

It was my way of saying that the PR is RTBC for its scope (moving the code). The other things I have reported are pre-existing issues anyway.

[quicksketch]

We should fix the problem @klonos described above, where Comment and Taxonomy modules will only take ownership of their views on new installs instead of existing ones. We need an update hook in both modules that just loads in their corresponding view config files, updates the module key, and then saves it.

[herbdool]

One question, should we include views.view.promoted in this PR?

[jenlampton]

Into Node module?

…

On Sat, Mar 2, 2019 at 10:49 AM Herb ***@***.***> wrote: One question, should we include views.view.promoted in this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[herbdool]

I think yes

[quicksketch]

It does seem weird that views.view.promoted stays within the Views module instead of Node. Could we apply this same change to that view as well?

[jenlampton]

I've moved the promoted view to the node module, and added an update hook to switch the module from views to node.

[quicksketch]

Looks like tests aren't passing because of views_install() referencing the promoted view config file.

[jenlampton]

Do you think I need to move the old update hook? edit: Oh, `views_install()` not `views.install`! checking...

[jenlampton]

Hm, views install doesn't reference the old config file:

/**
 * Implements hook_install().
 */
function views_install() {
  db_query("UPDATE {system} SET weight = 10 WHERE name = 'views'");
}

Here's what's in views.install:

/**
 * Replace the frontpage view with promoted.
 */
function views_update_1004() {
  // If the old frontpage view is still disabled, delete it.
  if (config_get('views.view.frontpage', 'disabled') === TRUE) {
    $config_frontpage = config('views.view.frontpage');
    $config_frontpage->delete();
  }
  // See if there is already a promoted view on this site.
  $config_promoted = config('views.view.promoted');
  if ($config_promoted->isNew()) {
    // If not, create the new promoted view and mark it disabled.
    $path = backdrop_get_path('module', 'views') . '/config/';
    $contents = file_get_contents($path . 'views.view.promoted.json');
    $data = json_decode($contents, true);
    $config_promoted->setData($data);
    $config_promoted->set('disabled', TRUE);
    $config_promoted->save();
  }
}

I'll try changing the module to node, but is it okay to leave this update in views module?

[quicksketch]

I'll try changing the module to node, but is it okay to leave this update in views module?

Yes. Though we shouldn't actually be doing this at all. This is why we copy the entire view into update hooks, and why we have an issue at #3347. Update hooks shouldn't be dependent upon the default configuration of a module, because that default configuration changes over time.

[quicksketch]

Thanks @jenlampton for the quick update! I've merged backdrop/backdrop#2464 into 1.x for 1.13.0. Thanks @BWPanda, @klonos, and @herbdool for your work here!