Problem/Motivation

Views should depend on all the things (config, modules, and content) that if they disappear would break the view.

Proposed resolution

Ensure that views can depend on:

  • Entity view modes used in any displays
  • Fields used in any displays
  • Fields used in any filters
  • Default values used in arguments
  • ... anything more...

This issue cannot handle two things, which have their own issues:

  1. dependencies on the config entities representing fields, for the fields that are listed in field views, see #2349553: Store entity field information in the views data instead
  2. dependencies on referenced content/config entities, for the Views entity area, see #2341357: Views entity area config is not deployable and missing dependencies instead

Remaining tasks

User interface changes

None

API changes

None

Original report by @alexpott

From #2267453-74: Views plugins do not store additional dependencies

I don't want to hold up this patch, but I was always under the impression that this patch would bring support for config entity dependencies as well? I.e. if a view lists entities in a certain view mode (the front page lists entities in 'teaser' view mode) it should depend on the view mode entities in question. If it filters to article nodes, it should depend on the article node type config entity. And so on. I suppose that is now out of scope? Or perhaps I'm mistaken and that's a different issue?

It's essential that we can figure out those config entity dependencies as well, because we'll need to make sure that cached views are invalidated when any of those config entities change. It is also essential to use more granular entity list cache tags in the future (i.e. if we know we're only listing 'Blog' nodes, then we can use a Blog node-specific list cache tag so that we invalidate as rarely as possible).

Files: 
CommentFileSizeAuthor
#84 views_config_content_deps-2372855-84.patch58.06 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,557 pass(es). View

Comments

Wim Leers’s picture

Priority: Major » Critical
Issue tags: +D8 cacheability, +Performance

I'd argue this is critical, because it's essential for having efficient cache invalidation of views.

damiankloip’s picture

It's essential that we can figure out those config entity dependencies as well

I'm sure we have talked about this numerous times, but I will say it again; This only helps us for entities that use config entities for their bundles, which all will not. So as mentioned before, relying on config entity dependencies from types is not an option. So just using node as an example is not the full problem space. Unless I miss something major here?

So based on that, I would say not critical, unless I am wrong of course, then yes.

Wim Leers’s picture

Good point! I'd forgotten about that. Except that we now can have content entity dependencies also: https://www.drupal.org/node/2364725. Hence this can address the full problem space, I think?

catch’s picture

You can still have entities that have multiple bundles defined in code - the equivalent to https://api.drupal.org/api/drupal/modules%21node%21node.api.php/function...

If we don't track dependencies for those, then they can disappear when the module is disabled, which is what resulted in #1017672: D6 to D7 update process permanently deletes comment bodies and other data, and throws fatal SQL errors during the 6-7 upgrade path.

Only the module defining the entity type is able to determine that though.

xjm’s picture

Issue tags: +D8 upgrade path
Wim Leers’s picture

Title: Add config entity dependencies to views » Add content & config entity dependencies to views
Parent issue: » #2323335: [meta] Improve Views' caching
Wim Leers’s picture

damiankloip (or other VDC folks), if you can provide some high-level pointers, I'll work on a patch for this.

dawehner’s picture

Wim Leers’s picture

alexpott’s picture

Issue summary: View changes

Updated the summary to describe the task better.

Wim Leers’s picture

Title: Add content & config entity dependencies to views » [PP-1] Add content & config entity dependencies to views
Issue summary: View changes
Wim Leers’s picture

Title: [PP-1] Add content & config entity dependencies to views » Add content & config entity dependencies to views
Issue summary: View changes

Actually, I think this is more of a soft blocker.

Wim Leers’s picture

Status: Active » Needs review
FileSize
3.52 KB

Here's an initial patch that takes care of the Page display plugin's optional menu link and the Bundle filter plugin. When added to the Frontpage view, that view's dependencies look like this:

dependencies:
  config:
    - menu.main
    - node.node_type.article
    - node.node_type.page
  module:
    - node
    - user

It'd be great if somebody could confirm I'm on the right track or not.

damiankloip’s picture

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -388,7 +388,14 @@ public static function preRenderFlattenData($form) {
-    return [];
+    $dependencies = [];
+
+    $provider = $this->getProvider();
+    if ($provider !== 'views') {
+      $dependencies['module'][] = $provider;
+    }
+
+    return $dependencies;

iirc, calculateDependencies() takes care of the provider, that's why we don't have it here :)

dawehner’s picture

Thank you for getting this started.

One thing we certainly have to look at is the implementation for the fieldapi field handler, called Field.php (do you need even more fields? FIELDS)
That handler would add both the field config entities, as well as

  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -388,7 +388,14 @@ public static function preRenderFlattenData($form) {
        * {@inheritdoc}
        */
       public function calculateDependencies() {
    -    return [];
    +    $dependencies = [];
    +
    +    $provider = $this->getProvider();
    +    if ($provider !== 'views') {
    +      $dependencies['module'][] = $provider;
    +    }
    +
    +    return $dependencies;
       }
     
    

    If we would drop the $provider != 'views' line, we could implement that functionality as part of the \Drupal\Core\Plugin\PluginBase ... I don't have problems with adding 'views' to be honest.

  2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -425,4 +425,18 @@ public function getPagerText() {
    +  public function calculateDependencies() {
    +    $dependencies = parent::calculateDependencies();
    +
    +    $menu = $this->getOption('menu');
    +    if ($menu['type'] === 'normal') {
    +      $dependencies['config'][] = 'menu.' . $menu['menu_name'];
    +    }
    +
    +    return $dependencies;
    +  }
    +
    

    Note: We do allow to configure to use contextual links for views ... so in case we do, we should add a dependency to contextual module, don't we?

  3. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
    @@ -73,4 +74,26 @@ public function query() {
    +    $bundle_entity_storage = \Drupal::entityManager()->getStorage($bundle_entity_type);
    

    Let's not be lazy here ...

  4. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
    @@ -73,4 +74,26 @@ public function query() {
    +      if ($bundle_entity instanceof ConfigEntityInterface) {
    +        $dependencies['config'][] = $this->entityTypeId . '.' . $bundle_entity_type . '.' . $bundle;
    +      }
    +      else {
    +        $dependencies['content'][] = $bundle_entity_type . ':' . $bundle_entity->uuid();
    +      }
    

    As alex said, getConfigDependencyName() is the way to go.

damiankloip’s picture

FileSize
798 bytes

Adding dependencies for the displays I think should be like this.

EDIT: Patch does do this, so I think we can still just remove the changes to PluginBase here? - Sorry, didn;t mean to get that tested!

dawehner’s picture

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

Added a few additional dependencies. Its pretty clear that some of the code is kinda redundant.

Note: in order to add dependencies for the corresponding entity fields, we do need #2349553: Store entity field information in the views data

Status: Needs review » Needs work

The last submitted patch, 17: 2372855-16.patch, failed testing.

Wim Leers’s picture

#17: does this issue need to be blocked on #2349553: Store entity field information in the views data then?

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.61 KB
3.29 KB

Mh k, this should be it to pass again.
I went through all the handlers for now, and hope I found all instances, which are possible to detect at the moment.

Wim Leers’s picture

FileSize
18.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,214 pass(es), 18 fail(s), and 20 exception(s). View
9.66 KB

Drupal should install again now. #17 introduced view mode config dependencies, but used view_display rather than view_mode, plus it used wrong view mode IDs (they need to be prefixed with the entity type ID). This was introducing so much duplication in the various RSS row plugins, that I added a new RssPluginBase abstract base class.

EDIT: dawehner and I worked on this in parallel. He wrote a different solution (no base class, but a trait), but it's still broken because the IDs it uses are still wrong.

Wim Leers’s picture

Issue summary: View changes

#21 shows how apparently we still have "view modes" and "view displays". I thought we had gotten rid of "view modes". Apparently, view modes exist only to be listed in the view modes/displays UI, essentially to dictate across all bundles of an entity type which view displays are available. Otherwise generic code like Views would not be able to list all nodes in a certain view display, because it'd be possible for bundle A to support view display X, but for bundle B to not support view display X.

This leads to an important question: how do we make sure that whenever a view display is modified (e.g. field F is shown before field G instead of vice versa), that the cached view is invalidated? We thought (or at least I did) we needed to depend on the corresponding view display config entities to make that happen.
But actually, we don't: rendering a view means rendering the listed entities in the configured view mode, which means the corresponding view display will be used for every listed entity, and rendering of an entity using a certain view display means that the cache tag associated with that view display will be set on the rendered entity.
This means only the render cache (Views output cache) will be invalidated. But this is correct: the Views results cache is unaffected by view display changes.

The last submitted patch, 20: 2372855-19.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: views_config_content_deps-2372855-20.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Working on fixing the test failures.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,443 pass(es), 18 fail(s), and 10 exception(s). View
4.03 KB

Fixed some failures, but not yet all.

In doing so, found a pretty big bug in the Page display: if it had a menu link, it would not be excluded from the parent link selection. :)

Status: Needs review » Needs work

The last submitted patch, 26: views_config_content_deps-2372855-26.patch, failed testing.

dawehner’s picture

Here is a quick review.

  1. +++ b/core/modules/node/src/Plugin/views/row/Rss.php
    @@ -28,7 +27,7 @@
    -class Rss extends RowPluginBase {
    +class Rss extends RssPluginBase {
    

    I like that, at least on an abstract level!

  2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -374,4 +374,17 @@ public function getCacheContexts() {
    +    $role_storage = \Drupal::entityManager()->getStorage('taxonomy_vocabulary');
    +    $vocabulary = $role_storage->load($this->options['vid']);
    

    let's fix the variable name.

  3. +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
    @@ -103,4 +103,20 @@ public function query() {
    +    $role_storage = \Drupal::entityManager()->getStorage('taxonomy_vocabulary');
    +    foreach (array_keys($this->options['vid']) as $vocabulary_id) {
    

    horrible variable name here as well.

  4. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -202,7 +202,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent);
    +          $menu_link = 'views_view:views.' . $form_state->get('view')->id() . '.' . $form_state->get('display_id');
    +          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent, $menu_link);
               $form['menu']['parent'] += array(
    

    Ha, but yeah we at least have the API prepared for it. Can we either move this to a different issue or add some dedicated test coverage?

  5. +++ b/core/modules/views/src/Plugin/views/row/EntityRow.php
    @@ -213,4 +213,20 @@ public function render($row) {
    +      ->getStorage('entity_view_display')
    +      ->load($this->entityTypeId . '.' . $this->options['view_mode']);
    
    +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
    @@ -0,0 +1,77 @@
    +   */
    +  public function calculateDependencies() {
    +    $dependencies = parent::calculateDependencies();
    +
    

    So we want to have entity_display_mode all the time?

  6. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
    @@ -0,0 +1,77 @@
    +    $view_modes = \Drupal::entityManager()->getViewModes('node');
    

    Let's use the entity type here ... 'node' is wrong here.

  7. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
    @@ -0,0 +1,77 @@
    +    $view_mode = \Drupal::entityManager()
    +      ->getStorage('entity_view_mode')
    +      ->load($this->entityTypeId . '.' . $this->options['view_mode']);
    +    if ($view_mode) {
    +      $dependencies['config'][] = $view_mode->getConfigDependencyName();
    +    }
    +
    

    The reason, why I went with the trait, is that #2322949: Implement generic entity link view field handlers will need that as well, and there we do have a field plugin. Believe me, sometimes even I, do know what I'm doing ...

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.09 KB
25.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,911 pass(es), 9 fail(s), and 10 exception(s). View
  1. Most of the remaining failures occur because doing *any* test with Views (even KernelTestBase tests) require the Views module to be installed, which requires its default config to be installed, which includes the default Views config of other modules. This includes amongst others the Node module's frontpage and glossary views. The glossary view requires the "main" menu to exist.
    In other words: installing Views on a site that has the Node module already installed requires the "main" menu to exist.
  2. Similarly, for e.g. AreaViewTest, we have a view that depends on another view, but they're always imported in alphabetical order rather than taking dependencies into account. This is because typically ViewTestData::createTestViews(get_class($this), array('views_test_config')); is used to import/install tested view config entities, which installs the views under test in *alphabetical* order, rather than in the specified order.

Both of the above are caused by using ViewTestData::createTestViews() instead of ConfigInstaller. That custom config install system does *not* take dependencies into account and hence can install view config entities in the wrong order (2), or not install dependencies on other config (1). Both amount to the same problem: dependencies are not handled.

But… how could that have been the case, if Views so far never specified config dependencies?

Hence this reroll introduces temporary work-arounds: it "improves" the current system by installing system module config first, instead of last, and by allowing protected $testViews = ['a', 'b'] to specify the test views to install in the order they should be installed.

Once we're satisfied with the config/content dependencies being added by all plugins, we should re-export all views and get rid of ViewTestData::createTestViews() altogether.

Finally, for e.g. CacheTagTest, we have a view that is filtering by bundle (node type), but the bundles are created *after* the view config entity is imported.

Wim Leers’s picture

FileSize
25.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,894 pass(es), 9 fail(s), and 10 exception(s). View
3.54 KB

Addressing #28

  1. :)
  2. Done.
  3. Done.
  4. I'm afraid we can't do it elsewhere, because without this, the asserted options would be wrong. We could of course fix it in a child issue.
  5. Heh, yes, fixed. I wasn't testing this one, so it slipped through the cracks.
  6. Good catch, fixed.
  7. I'm totally fine with a trait! I'm sorry I didn't make that clearer in #21. I have no opinion about this at all, I'll go with whatever you want. :) I just happened to have cross-posted a different approach with you.
    (Not yet implemented here though, I have more changes locally. I will do it in one of the next rerolls unless you beat me to it.)

The last submitted patch, 29: views_config_content_deps-2372855-29.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: views_config_content_deps-2372855-30.patch, failed testing.

dawehner’s picture

Once we're satisfied with the config/content dependencies being added by all plugins, we should re-export all views and get rid of ViewTestData::createTestViews() altogether.

I'm fine with doing that, as long we get rid of EntityManager::getDefinition()

Wim Leers’s picture

From an IRC discussion with damiankloip and dawehner: We should keep ViewsTestData::createTestViews(), but make it apply the same dependency-handling logic as ConfigInstaller::createConfiguration() has. Otherwise we'd end up installing every test view for any test that only needs a single test view, plus all their dependencies. That would slow down tests quite a bit.

Wim Leers’s picture

So, as a next step, I looked at the failures in FilterEntityBundleTest. They show the same problem that CacheTagTest showed:

a view that is filtering by bundle (node type), but the bundles are created *after* the view config entity is imported.

For CacheTagTest, that filtering was actually unnecessary, so I was able to "fix" the test by removing that filtering. In this case, there's no easy way out like that.

The problem here goes much, much deeper:

  protected function setUp() {
    parent::setUp();

    $this->drupalCreateContentType(array('type' => 'test_bundle'));
    $this->drupalCreateContentType(array('type' => 'test_bundle_2'));

The parent::setUp() calls ViewsTestData::createTestViews(), which imports the test views. But then the bundles don't exist yet… So we must ensure the bundles *do* exist already when we create the test views.

You'd think that changing it to this:

  protected function setUp() {
    $this->drupalCreateContentType(array('type' => 'test_bundle'));
    $this->drupalCreateContentType(array('type' => 'test_bundle_2'));

    parent::setUp();

solves the problem. But that doesn't work, unfortunately. Because drupalCreateContentType() needs WebTestBase::setUp() to already have been executed.

So now we've got a chicken and egg situation. And no clean way to solve it.

Any ideas?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
27.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,969 pass(es), 7 fail(s), and 10 exception(s). View
3.52 KB

I discussed the question in #35 with dawehner, damiankloip & Alex Pott on IRC last night: add a helper method invoked by ViewsTestData::createTestViews() that is called before the test views are actually imported. But turns out that possibility already exists: ViewTestBase::setUp() allows a FALSE parameter to be passed in, which then skips the importing of the test views. Which allows us to do:

parent::setUp(FALSE);

// Create dependencies.
//…

// Manually trigger importing of test views.
ViewTestData::createTestViews(get_class($this), array('views_test_config'));

So I did just that :)


This fixes both FilterEntityBundleTest + DisplayPathTest.

Status: Needs review » Needs work

The last submitted patch, 36: views_config_content_deps-2372855-36.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,168 pass(es), 2 fail(s), and 2 exception(s). View
5.34 KB

All fixed except one test (2 failures, 2 exceptions).

Status: Needs review » Needs work

The last submitted patch, 38: views_config_content_deps-2372855-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,069 pass(es), 1 fail(s), and 0 exception(s). View
1.37 KB

Let's just fix it.

dawehner’s picture

Issue tags: +Ghent DA sprint

Let's add a proper tag.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/argument_validator/Entity.php
@@ -211,4 +212,22 @@ protected function validateEntity(EntityInterface $entity) {
+      $key = $bundle_entity instanceof ConfigEntityInterface ? 'config' : 'content';

+++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
@@ -73,4 +74,22 @@ public function query() {
+      $key = $bundle_entity instanceof ConfigEntityInterface ? 'config' : 'content';

EntityType now has getConfigDependencyKey() - see #2390615: Add method to determine config dependency key depending on entity type

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
31.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,065 pass(es), 1 fail(s), and 0 exception(s). View
1.61 KB

Done.

The last submitted patch, 40: 2372855-40.patch, failed testing.

Wim Leers’s picture

FileSize
30.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,071 pass(es). View
587 bytes

#40 fixed a bug in a test View, which then caused a corresponding assertion to fail. Now fixed.

Wim Leers’s picture

Issue summary: View changes
Related issues: +#2349553: Store entity field information in the views data
FileSize
31.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,174 pass(es). View
2.17 KB

And clean-up, of both loose ends in the patch and the IS.

The last submitted patch, 43: views_config_content_deps-2372855-43.patch, failed testing.

amateescu’s picture

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -374,4 +374,17 @@ public function getCacheContexts() {
+    $dependencies['config'][] = $vocabulary->getConfigDependencyName();

+++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
@@ -103,4 +103,20 @@ public function query() {
+        $dependencies['config'][] = $vocabulary->getConfigDependencyName();

+++ b/core/modules/user/src/Plugin/views/access/Role.php
@@ -92,4 +92,20 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
+        $dependencies['config'][] = $role->getConfigDependencyName();

+++ b/core/modules/views/src/Plugin/views/area/View.php
@@ -151,4 +151,19 @@ public function isEmpty() {
+      $dependencies['config'][] = $view->getConfigDependencyName();

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -425,4 +426,19 @@ public function getPagerText() {
+      $dependencies['config'][] = $menu_entity->getConfigDependencyName();

+++ b/core/modules/views/src/Plugin/views/row/EntityRow.php
@@ -213,4 +213,20 @@ public function render($row) {
+      $dependencies['config'][] = $view_mode->getConfigDependencyName();

+++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
@@ -0,0 +1,77 @@
+      $dependencies['config'][] = $view_mode->getConfigDependencyName();

There are still quite a few places where we need to use the new method :)

Otherwise, the patch looks good to me.

Wim Leers’s picture

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

Done.

Wim Leers’s picture

FileSize
34.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,070 pass(es). View
1.71 KB

Forgot a bit.

The last submitted patch, 49: views_config_content_deps-2372855-49.patch, failed testing.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Awesome! :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

We need to confirm that we have test coverage here.

Wim Leers’s picture

FileSize
44.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,072 pass(es), 7 fail(s), and 0 exception(s). View
14.35 KB

Discussed with Alex Pott how to write test coverage for the many new calculateDependencies() methods. The problem with testing these in isolation (i.e. as unit tests) is extremely laborious: it requires mocking an insane amount of dependencies, because Views depends reaches out to almost every other subsystem.

Therefore, we decided it would be okay for this test coverage to be implemented wherever it would be easiest to add, just like was done for #2267453: Views plugins do not store additional dependencies. The test coverage her doesn't have to be perfect. We basically just want to make sure we don't easily regress here.

Added tests for:

  1. core/modules/field/src/Plugin/views/field/Field
  2. core/modules/views/src/Plugin/views/row/RssPluginBase — tested via the frontpage view, because it's the only view in core that lists multiple view modes
  3. core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid — here I discovered that we should also have added dependencies on any referenced taxonomy terms, yay for test coverage! :)
  4. core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData
  5. core/modules/user/src/Plugin/views/access/Role
  6. core/modules/views/src/Plugin/views/area/View
  7. NOT: core/modules/views/src/Plugin/views/argument_validator/Entity — has zero views using it, hence zero test coverage, hence I did not add test coverage for this one
  8. core/modules/views/src/Plugin/views/display/Page
  9. core/modules/views/src/Plugin/views/filter/Bundle
  10. NOT: core/modules/views/src/Plugin/views/row/EntityRow — has zero views using it, hence zero test coverage, hence I did not add test coverage for this one

I think I found a config schema problem with NodeTermData. The schema looks like this:

views.relationship.node_term_data:
  type: views_relationship
  label: 'Taxonomy term'
  mapping:
    vids:
      type: sequence
      label: 'Vocabularies'
      sequence:
        - type: string
          label: 'Vocabulary'

The sole example in core is in a test view, and it looks like this:

      relationships:
        term_node_tid:
          field: term_node_tid
          id: term_node_tid
          admin_label: 'Term #1'
          table: node
          vids:
            tags: ''
          plugin_id: node_term_data
        term_node_tid_1:
          field: term_node_tid
          id: term_node_tid_1
          admin_label: 'Term #2'
          table: node
          vids:
            tags: ''
          plugin_id: node_term_data

Two NodeTermData relationships, but note that under the vids key, we don't find a sequence, but a mapping. And, worse yet, in the tests, we modify the value of that mapping, so this was definitely intentional at some point:

    // Change the view to test relation limited by vocabulary.
    \Drupal::config('views.view.test_taxonomy_node_term_data')
      ->set('display.default.display_options.relationships.term_node_tid.vids.tags', 'views_testing_tags')
      ->save();

Fixed all that, by:

  1. implementing ::submitOptionsForm(), to normalize the data
  2. fixing the test to actually configure a sequence instead of a mapping
  3. fixing the calculateDependencies() method now that the data actually adheres to the schema

Whew. This took many, many hours instead of the 30 minutes or so I expected…

Status: Needs review » Needs work

The last submitted patch, 54: views_config_content_deps-2372855-54.patch, failed testing.

Gábor Hojtsy’s picture

@Wim: that is actually a valid sequence. There is no requirement for a sequence to be numbered. The only difference between a sequence and a mapping is for a mapping you exactly know all the keys that there gonna be. For a sequence you don't know the exact keys. That's it. We could have named it better maybe. So the cited config snippet is valid according to the cited config schema. Examples of string keyed sequences include things keyed by plugin id for example, such as #2391925-5: Ensure page_manager config schema is valid where display variants are a sequence even though it is string indexed. But there can be any number of them and we don't know the keys of them.

Wim Leers’s picture

Wow, that is tremendously confusing then :(

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
45.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,087 pass(es), 1 fail(s), and 0 exception(s). View
1.7 KB

That last fail was due to something similar.

Should be green now, and needs review.

Status: Needs review » Needs work

The last submitted patch, 58: views_config_content_deps-2372855-58.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
45.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,093 pass(es). View
836 bytes

LOL.

Wim Leers’s picture

So the reason for the failure in #58, fixed in #60, is that the config schema dictates that value should contain a sequence of strings, even though in the case of taxonomy term filter, we really are dealing with integers (taxonomy term IDs). So we should adjust the schema. Opened a follow-up for that, since it's out of scope here: #2392263: Sequence subtyping cannot override item type in config schema, views taxonomy term filter schema incorrect.

dawehner’s picture

  1. +++ b/core/modules/comment/src/Plugin/views/row/Rss.php
    @@ -62,18 +47,10 @@ public function preRender($result) {
    +  public function buildOptionsForm_summary_options() {
    +    $options = parent::buildOptionsForm_summary_options();
         $options['title'] = $this->t('Title only');
         $options['default'] = $this->t('Use site default RSS settings');
    

    Wow, I did forgot about it, totally!

  2. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_node_term_data.yml
    @@ -57,16 +57,14 @@ display:
               id: term_node_tid
               admin_label: 'Term #1'
               table: node
    -          vids:
    -            tags: ''
    +          vids: {}
               plugin_id: node_term_data
             term_node_tid_1:
               field: term_node_tid
               id: term_node_tid_1
               admin_label: 'Term #2'
               table: node
    -          vids:
    -            tags: ''
    +          vids: {}
               plugin_id: node_term_data
           sorts:
             nid:
    

    So one thing we don't do at all yet is to update the dependencies of the existing test views ... I would assume there are some we have to update, especially for things like view modes?

Wim Leers’s picture

So one thing we don't do at all yet is to update the dependencies of the existing test views ... I would assume there are some we have to update, especially for things like view modes?

We discussed this two days ago, and IIRC Alex Pott said we shouldn't update all existing (test) views in this issue, because that'd cause conflicts in many other patches.

dawehner’s picture

I would agree for test views, but I don't agree for actual used default views.
On top of that I also don't see any follow up for that yet.

Wim Leers’s picture

Talked to Alex Pott again and he re-confirmed that we indeed want to do the re-exporting of views in a follow-up issue. Opened that: #2392601: Re-export views now that all views now that they list content & config dependencies.

(I honestly don't care, so if you disagree, please talk to Alex.)

Other than the re-exporting, any other remarks? Do the tests I added in #54—#60 look good?

tim.plunkett’s picture

The tests seem good.

Some nits:

  1. +++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
    @@ -22,7 +21,7 @@
    -class Rss extends RowPluginBase {
    +class Rss extends RssPluginBase {
    

    Nice refactor.

  2. +++ b/core/modules/node/src/Tests/Views/NodeLanguageTest.php
    @@ -41,11 +42,12 @@ class NodeLanguageTest extends NodeTestBase {
    -    parent::setUp();
    +    parent::setUp(FALSE);
    
    +++ b/core/modules/node/src/Tests/Views/NodeTestBase.php
    @@ -22,10 +22,12 @@
    +  protected function setUp($import_test_views = TRUE) {
    ...
    +    if ($import_test_views) {
    

    Neat trick.

  3. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyIndexTidUiTest.php
    @@ -96,6 +103,22 @@ public function testFilterUI() {
    +      ]
    

    Missing trailing comma.

  4. +++ b/core/modules/views/src/Plugin/views/area/View.php
    @@ -151,4 +151,19 @@ public function isEmpty() {
    +    list($view_id, $display_id) = explode(':', $this->options['view_to_insert']);
    

    We don't use $display_id here, no reason to have it in the list(). Also, might as well add , 2 to the end of that explode()

  5. +++ b/core/modules/views/src/Tests/Entity/FilterEntityBundleTest.php
    @@ -70,6 +73,19 @@ protected function setUp() {
    +        'node'
    +      ]
    

    Trailing commas.

Wim Leers’s picture

FileSize
45.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,186 pass(es). View
2.01 KB

Thanks for the review! Fixed all nits.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Its a little bit sad that we change the behaviour in one of many many many cases, but well, this itself is a step forward.

Beside from that its a great improvement.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -374,4 +374,23 @@ public function getCacheContexts() {
    +    $vocabulary = \Drupal::entityManager()->getStorage('taxonomy_vocabulary')
    ...
    +    $term_storage = \Drupal::entityManager()->getStorage('taxonomy_term');
    

    Let inject here too

  2. +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
    @@ -103,4 +113,20 @@ public function query() {
    +    $vocabulary_storage = \Drupal::entityManager()->getStorage('taxonomy_vocabulary');
    

    Lets inject the taxonomy vocabulary storage.

  3. +++ b/core/modules/user/src/Plugin/views/access/Role.php
    @@ -92,4 +92,20 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
    +    $role_storage = \Drupal::entityManager()->getStorage('user_role');
    

    Lets inject the role storage

  4. +++ b/core/modules/views/src/Plugin/views/area/View.php
    @@ -151,4 +151,19 @@ public function isEmpty() {
    +      $view = \Drupal::entityManager()->getStorage('view')->load($view_id);
    

    $this->viewStorage

  5. +++ b/core/modules/views/src/Plugin/views/argument_validator/Entity.php
    @@ -211,4 +212,22 @@ protected function validateEntity(EntityInterface $entity) {
    +    $bundle_entity_type = \Drupal::entityManager()->getDefinition($entity_type_id)->getBundleEntityType();
    +    $bundle_entity_storage = \Drupal::entityManager()->getStorage($bundle_entity_type);
    

    $this->entityManager

  6. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -202,7 +202,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
             if (\Drupal::moduleHandler()->moduleExists('menu_ui')) {
    ...
    +          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent, $menu_link);
    
    @@ -425,4 +426,19 @@ public function getPagerText() {
    +      $menu_entity = \Drupal::entityManager()->getStorage('menu')->load($menu['menu_name']);
    

    Lets inject all of this too

  7. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
    @@ -73,4 +74,21 @@ public function query() {
    +    $bundle_entity_storage = \Drupal::entityManager()->getStorage($bundle_entity_type);
    

    Lets inject the entity manager

  8. +++ b/core/modules/views/src/Plugin/views/row/EntityRow.php
    @@ -213,4 +213,20 @@ public function render($row) {
    +    $view_mode = \Drupal::entityManager()
    

    $this->entityManager

  9. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
    @@ -0,0 +1,77 @@
    +    $view_mode = \Drupal::entityManager()
    +      ->getStorage('entity_view_mode')
    +      ->load($this->entityTypeId . '.' . $this->options['view_mode']);
    

    We should inject this too...

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
57.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,393 pass(es), 1 fail(s), and 0 exception(s). View
20.04 KB

Et voilà!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 70: views_config_content_deps-2372855-70.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
58.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,405 pass(es). View
1.11 KB

Gah.

damiankloip’s picture

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
@@ -53,45 +53,6 @@ display:
-      filters:
-        type:
-          id: type
-          table: node_field_data
...
-          relationship: none
-          group_type: group
-          admin_label: ''
-          operator: in
-          value:
-            page: page

So one thing I am wondering, this is removed to just get the tests to pass really?!And this was just something that was not removed when we ripped out the bundle cache tag logic from views (my fault!).

So what happens when we then need this back? Also, the creation of bundles is still in the test too.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -405,6 +405,17 @@ public function getTypedData();
    +  public function getConfigDependencyKey();
    

    We're also adding yet another method to entities :/

  2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -425,4 +475,19 @@ public function getPagerText() {
    +    if ($menu['type'] === 'normal') {
    

    Do we need a @todo here when we fully support all menu things?

  3. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
    @@ -73,4 +113,21 @@ public function query() {
    +    $bundle_entity_storage = \Drupal::entityManager()->getStorage($bundle_entity_type);
    

    Shouldn't that be using the injected manager now?

Also, not sure RTBC'ing your own patch after 20kb of changes is the done thing? :)

dawehner’s picture

Also, not sure RTBC'ing your own patch after 20kb of changes is the done thing? :)

Agreed

Do we need a @todo here when we fully support all menu things?

Well ... technically I can't think that we need another one. local actions/tasks and contextual links can't be added to a menu itself.

pfrenssen’s picture

Issue summary: View changes
damiankloip’s picture

So all those menu dependencies are satisfied then? Why do we need to check the type?

dawehner’s picture

So all those menu dependencies are satisfied then? Why do we need to check the type?

Well ... for those other types we don't use menu entities ... we just want to export the config dependency, in case we have to.

damiankloip’s picture

Sure, that makes sense. Can you tell I have not really looked at ANY menu changes ;)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
58.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,528 pass(es). View
862 bytes

#73: The alternative is to fix the test to create the bundles early enough, *before* the view is imported. We could do that, but these filters on the view are simply unnecessary for doing the tests that we care about, so it's more logical to just remove those filters. And finally, the reason we don't remove the bundles, is that they're fine to keep. We need to create *some* bundle anyway, and the fact that this happens to create two bundles doesn't hurt.
(So: the filters *do* hurt, the multiple bundles instead of a single bundle *don't* hurt.)

#74/#75: the only reason I RTBC'd after 20K of changes, is that it's 20K of boilerplate AKA super trivial changes. Otherwise I wouldn't even have considered it. But you're right, I shouldn't have. Sorry. Won't happen again.

#74.1: yep, but that already was there when it was RTBC'd in #68. It's a sad necessity. Alex Pott suggested/approved it.
#74.2: answered by dawehner in #75/#77/#78/#79.
#74.3: good catch, fixed!

#76: thanks :)

dawehner’s picture

Status: Needs review » Needs work

Did another review here ...

  1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -28,6 +32,54 @@ class TaxonomyIndexTid extends ManyToOne {
    +    $this->is_handler = TRUE;
    
    +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
    @@ -22,6 +24,43 @@
    +    $this->is_handler = TRUE;
    
    +++ b/core/modules/user/src/Plugin/views/access/Role.php
    @@ -32,6 +34,43 @@ class Role extends AccessPluginBase {
    +    $this->is_handler = TRUE;
    

    it is not obvious why we need this here? C&p from HandlerBase

  2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -425,4 +475,19 @@ public function getPagerText() {
    +      $menu_entity = \Drupal::entityManager()->getStorage('menu')->load($menu['menu_name']);
    

    We would also inject that here.

  3. +++ b/core/modules/views/src/Plugin/views/row/RssPluginBase.php
    @@ -0,0 +1,116 @@
    +    $view_modes = \Drupal::entityManager()->getViewModes($this->entityTypeId);
    

    Let's use the injected object.

  4. +++ b/core/modules/views/src/Tests/ViewTestData.php
    @@ -45,9 +45,10 @@ public static function createTestViews($class, array $modules) {
    -        foreach ($file_storage->listAll('views.view.') as $config_name) {
    -          $id = str_replace('views.view.', '', $config_name);
    -          if (in_array($id, $views)) {
    +        $available_views = $file_storage->listAll('views.view.');
    +        foreach ($views as $id) {
    +          $config_name = 'views.view.' . $id;
    +          if (in_array($config_name, $available_views)) {
                 $storage
    

    the new logic does indeed make a little bit more sense.

  5. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_page_display_menu.yml
    index 4399c42..802f06f 100644
    --- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
    
    --- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
    
    +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tag_cache.yml
    @@ -57,47 +57,6 @@ display:
    -      filters:
    -        type:
    -          id: type
    -          table: node_field_data
    -          field: type
    -          relationship: none
    -          group_type: group
    -          admin_label: ''
    -          operator: in
    -          value:
    -            page: page
    -          group: 1
    -          exposed: false
    -          expose:
    -            operator_id: ''
    -            label: ''
    -            description: ''
    -            use_operator: false
    -            operator: ''
    -            identifier: ''
    -            required: false
    -            remember: false
    -            multiple: false
    -            remember_roles:
    -              authenticated: authenticated
    -            reduce: false
    -          is_grouped: false
    -          group_info:
    -            label: ''
    -            description: ''
    -            identifier: ''
    -            optional: true
    -            widget: select
    -            multiple: false
    -            remember: false
    -            default_group: All
    -            default_group_multiple: {  }
    -            group_items: {  }
    -          plugin_id: bundle
    -          entity_type: node
    -          entity_field: type
    

    wat?

  6. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -1129,6 +1129,12 @@ public function calculateDependencies() {
        */
    +  public function getConfigDependencyKey() {
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    
    +++ b/core/modules/views_ui/tests/src/Unit/ViewUIObjectTest.php
    @@ -42,7 +42,7 @@ public function testEntityDecoration() {
           // dependency management.
    -      if (!in_array($reflection_method->getName(), ['isNew', 'isSyncing', 'isUninstalling', 'getConfigDependencyName', 'calculateDependencies'])) {
    +      if (!in_array($reflection_method->getName(), ['isNew', 'isSyncing', 'isUninstalling', 'getConfigDependencyKey', 'getConfigDependencyName', 'calculateDependencies'])) {
    

    Lazyness ... you can easily just write $this->storage->getConfigDependencyKey() ... and no, the other cases of config relation methods are a bug as well ... Most of the decorator functions got implemented

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch views_config_content_deps-2372855-82.patch. Unable to apply patch. See the log in the details link for more information. View
4.98 KB
  1. Wow, so I did mess up the trivial changes/boilerplate… Sorry guys :( Fixed!
  2. Wim--; fixed.
  3. Done.
  4. :)
  5. It's not a strange as it seems at first sight — see #73 and the reply to that in #80.
  6. Indeed. But indeed, the reason I did it, is because the other config method doesn't do it. I've fixed all of them now.

Status: Needs review » Needs work

The last submitted patch, 82: views_config_content_deps-2372855-82.patch, failed testing.

Wim Leers’s picture

FileSize
58.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,557 pass(es). View

I uploaded the interdiff as the patch. Sorry about that.

alexpott’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  1. +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
    @@ -49,6 +49,20 @@ public function testFrontPage() {
    +    // Tests \Drupal\node\Plugin\views\row\RssPluginBase::calculateDependencies().
    +    $expected = [
    +      'config' => [
    +        'core.entity_view_mode.node.rss',
    +        'core.entity_view_mode.node.teaser',
    +      ],
    +      'module' => [
    +        'node',
    +        'user',
    +      ],
    +    ];
    +    $this->assertIdentical($expected, $view->calculateDependencies());
    

    Yay!

  2. +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -202,7 +248,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent);
    +          $menu_link = 'views_view:views.' . $form_state->get('view')->id() . '.' . $form_state->get('display_id');
    +          $form['menu']['parent'] = \Drupal::service('menu.parent_form_selector')->parentSelectElement($menu_parent, $menu_link);
    

    Can we get a followup to inject this service now we have all the boilerplate.

  • alexpott committed bb4367b on 8.0.x
    Issue #2372855 by Wim Leers, dawehner: Add content...
Wim Leers’s picture

Can we get a followup to inject this service now we have all the boilerplate.

Done: #2394021: Inject menu parent form selector and module handler services into Views' Page display plugin.

damiankloip’s picture

(So: the filters *do* hurt, the multiple bundles instead of a single bundle *don't* hurt.)

But if we are removing the bundle filters, we should have also just removed the creation of the bundles too maybe? IF/WHEN bundle level cache tags come back, these filters will be needed again anyway, so I would have rather it was just fixed in this issue and left alone, but hey, I'm only a views maintainer.. :) I'll take your word for it that it should work ok when we do need to do that.

Wim Leers’s picture

If you want me to get rid of the second bundle, I'll do that. Do you want me to do it in a follow-up?

yched’s picture

Getting seemingly unrelated test fail on AccessRoleTest in #2381777: Unify setValue() implementations in ItemList & FieldItemList.

The test is green at home, both on HEAD and with the patch over there applied, so it might be a random fail.

The testbot reported a fail on :

    $display['display_options']['access']['options']['role'] = array(
      $this->normalRole => $this->normalRole,
      'anonymous' => 'anonymous',
    );
    $view->save();
    $expected = [
      'config' => [
        'user.role.anonymous',
        'user.role.' . $this->normalRole,
      ],
      'module' => ['user'],
    ];
    $this->assertIdentical($expected, $view->calculateDependencies());

Because the order of the user.role.* dependencies differ in $expected and in the actual calculated dependencies.

\Drupal\user\Plugin\views\access\Role::calculateDependencies() just adds dependencies in the same order as the options.
So it looks weird that we assign "normalRole then anonymous" in the view's options, but expect the dependencies to come up the other way around ?

dawehner’s picture

Well, given that the order of config dependencies should not matter, the test should also try to avoid checking for it, right?

Status: Fixed » Closed (fixed)

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