The View entity calculates all the dependencies each display introduces on config save. The current code results in dependencies being calculated many times for the same handler or plugin. Also the View config entity is exposed to internals of the display plugin it should not be.

There are multiple examples of test coverage - perhaps the most relevant is Drupal\views\Tests\Entity\ViewEntityDependenciesTest

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
4.47 KB

The patch attached moves the responsibility for calculating the dependencies each display adds to the display plugin. This moves views another step towards using EntityWithPluginCollectionInterface and having even less custom code in View::calculateDependencies().

The comment to add the dependency is removed because we have #2368767: Implement calculateDependencies() in ArgumentPluginBase to get dependencies from validator and default plugins which is taking care of that.

dawehner’s picture

The patch attached moves the responsibility for calculating the dependencies each display adds to the display plugin.

I really like that.

alexpott’s picture

dawehner’s picture

Issue tags: +Needs tests

... it would be nice, if we would have tests for that, I'd guess.

alexpott’s picture

Issue tags: -Needs tests
FileSize
6.89 KB
11.17 KB

Added a display level test to ViewEntityDependenciesTest

alexpott’s picture

FileSize
1.93 KB
11.4 KB

On the advice of @dawehner I added a code comment to explain that each display only calculates dependencies for plugins and handlers it configures.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC

Nice usage of array_walk!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2413753.6.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
11.02 KB
2.46 KB

reroll, removed one use statement and changed @return of getAllPlugins().

Status: Needs review » Needs work

The last submitted patch, 9: 2413753-9.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
11.05 KB
1.22 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is still RTBC IMHO.

Wim Leers’s picture

This patch looks awesome!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 161d80f on 8.0.x
    Issue #2413753 by alexpott, olli: Views recalculating the dependencies...

Status: Fixed » Closed (fixed)

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