Problem/Motivation

Views collects dependencies for a view in all sorts of ways, eg. handlers used under displays. Unfortunately the module providing the display itself is not added as a dependency, although it may very well be an optional module. Eg. user module provides two views that have block displays. The block display cannot be validated as configuration unless the block module is enabled. The display then is merely "optional" or in other words plain broken.

Found this in #2183983: Find hidden configuration schema issues.

Proposed resolution

1. Make the view inherit the provider of displays as dependencies of the view config entity.
2. Add missing provider info on block, rest and entity_reference displays in shipped config. There are two other displays in tests, but those are not referenced in shipped config.

The resolution itself will not fix the dependency problem as configuration is still imported even if internal dependencies are not met. The configuration system requires that configuration is in containing modules that are themselves dependent on all modules that their configuration depends on. That means the crucial node, user and comment module views affected in this patch will *also* need to move to a place where block module is enabled (naturally to standard profile). That will be in #2309715: Several views still say they depend on block module but not anymore.

Remaining tasks

Reviews. Commit.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#15 2309247-views-displays-15.patch11.83 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,313 pass(es). View
#15 interdiff.txt1.14 KBGábor Hojtsy
#13 interdiff.txt1.09 KBGábor Hojtsy
#13 2309247-views-displays-13.patch10.69 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,060 pass(es). View
#8 2309247-views-displays-8.patch10.68 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,901 pass(es), 16 fail(s), and 0 exception(s). View
#8 interdiff.txt555 bytesGábor Hojtsy
#5 2309247-views-displays-5.patch10.14 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,006 pass(es), 16 fail(s), and 0 exception(s). View
#5 interdiff.txt3.36 KBGábor Hojtsy
#4 interdiff.txt7.26 KBGábor Hojtsy
#4 2309247-views-displays-4.patch6.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,903 pass(es), 15 fail(s), and 0 exception(s). View
#1 2309247-views-displays-1.patch5.11 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Comments

Gábor Hojtsy’s picture

FileSize
5.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 1: 2309247-views-displays-1.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
6.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,903 pass(es), 15 fail(s), and 0 exception(s). View
7.26 KB

Erm, that is not the display instance, so we cannot invoke methods on it like that. However, looking at the code above (and trying it out as well :), views already saves the provider of the display itself on the display. This code on addDisplay():

    $display_options = array(
      'display_plugin' => $plugin_id,
      'id' => $id,
      // Cast the display title to a string since it is an object.
      // @see \Drupal\Core\StringTranslation\TranslationWrapper
      'display_title' => (string) $title,
      'position' => $id === 'default' ? 0 : count($this->display),
      'provider' => $plugin['provider'],
      'display_options' => array(),
    );

So we can just take that provider when building dependencies, much like the handler providers, but this one is top level on the display. We can take it right from the data.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +Needs tests
FileSize
3.36 KB
10.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,006 pass(es), 16 fail(s), and 0 exception(s). View

Two more display types found that have shipped config. Also updated issue summary.

tim.plunkett’s picture

The last submitted patch, 4: 2309247-views-displays-4.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
555 bytes
10.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,901 pass(es), 16 fail(s), and 0 exception(s). View

Yeah those are even more dependencies.

Looks like this fails on schema only, which is great. (Although it does not have its own tests yet, not sure how best to test this, some help with that would be really appreciated).

Gábor Hojtsy’s picture

Title: Views do not collect dependencies from displays » View do not depend on modules providing their displays

Better title since this limits itself on the modules providing the display itself, not deeper than that.

catch’s picture

Issue tags: +D8 upgrade path

Per #2267453-5: Views plugins do not store additional dependencies I think this might need to be critical. Tagging upgrade path for now anyway since we'll have to update views already in the system with the proper dependencies.

The last submitted patch, 5: 2309247-views-displays-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2309247-views-displays-8.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,060 pass(es). View
1.09 KB

Duh, added the schema at the wrong level. It should be on the display level, not display options of course.

Gábor Hojtsy’s picture

So @alexpott says its great that we improve the dependency detection, and this should get in. BUT this will not affect whether this config is installed or not. Quoting him: so the module should depend on all its dependencies - if it creates a view with a block display - it depends on block; the fact that we don't have this in the .info.yml file is just a lie; [...]. being honest about our dependencies is painful. We definitely don't want node, user and comment modules to depend on block module, so moving those views to the standard profile seems to be the only sensible choice.

Gábor Hojtsy’s picture

FileSize
1.14 KB
11.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,313 pass(es). View

Now with updated tests proving the provider of the display ends up in config entity dependencies. This will NOT result in the config entity not being imported when the dependency is not met, we'll still need to move the files. I'm not convinced we should move the files here, since there is already a lot going on in this patch, but we can do it of course. I'll upload another patch to see how that fares.

catch’s picture

While the standard profile 'depends' on the modules it specifies, it actually doesn't, see #820054: Add support for recommends[] and fix install profile dependency special casing and others. Although I guess the dependencies will be checked when a module is uninstalled, and then either cleaned up or the config entities deleted, so maybe that's OK anyway?

Gábor Hojtsy’s picture

Title: View do not depend on modules providing their displays » Views do not depend on modules providing their displays
Issue summary: View changes
Issue tags: -Needs tests

@catch: yeah that may require more discussion, discussed the moving again with @alexpott, and we agreed it should be a followup. So this patch is complete, has tests and all the things it needs to :) Need reviews then. Updated issue summary. Opened #2309715: Several views still say they depend on block module but not anymore for the moving. I'll jump right there once this is in and that should finally help relieve a lot of the fails in #2183983: Find hidden configuration schema issues. :)

moshe weitzman’s picture

If a module happens to ship with a View that takes advantage of block module, it does not mean that the module *depends* on block. Moving things to standard profile is a convenience that Contrib doesn't have. I'd rather not see that workaround here - or perhaps I am misunderstanding.

Gábor Hojtsy’s picture

@Moshe: But are config entities supposed to contain fragments for optional things? #2212081: Remove views optional handler handling recently landed that said a firm no. This issue does not want to move the views, I opened another one for it, but looks like you are saying it is also false to say a view that has a display provided by block module depends on block module?!

moshe weitzman’s picture

Correct. It is not a *dependency*. It is a *suggestion* or a recommendation. Composer uses the term suggestion for this sort of relationship - https://getcomposer.org/doc/04-schema.md#suggest.

The problem with being strict here is that up and coming modules have no chance of growing. Lets say I spot a cool new module that has almost no usage. They have a great API and it only takes me a few lines in my module to support it. If my module has to *depend* on the new module, then I create a new hassle and decision point for anyone updating to latest version. I might even choose not to support the new module so as to keep my dependency list low.

The patch here looks fine. It makes sense for each View to be explicit about its dependencies. I'm more concerned when dependency checks are applied at higher levels like modules.

Gábor Hojtsy’s picture

Ok, let's get this one in then, so we can continue discussing moving it on the other issue. This has tests, it does all the fixes, etc. Any problems with the patch?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is progress for now.

damiankloip’s picture

This can go in I guess. Could well just be replaced straight away with https://www.drupal.org/node/2267453 though? Depending on how that issue goes. If the provider and dependencies still get added separately it will make sense, otherwise it will not.

dawehner’s picture

Yeah, but this one at least already adapts many of the YML files itself.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • catch committed 718a47d on 8.0.x
    Issue #2309247 by Gábor Hojtsy: Fixed Views do not depend on modules...
Gábor Hojtsy’s picture

Let's do the hard dependency or suggestion discussion at #2309715: Several views still say they depend on block module but not anymore.

Status: Fixed » Closed (fixed)

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