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

CommentFileSizeAuthor
#11 interdiff.txt1.22 KBolli
#11 2413753-11.patch11.05 KBolli
#9 interdiff.txt2.46 KBolli
#9 2413753-9.patch11.02 KBolli
#6 2413753.6.patch11.4 KBalexpott
#6 5-6-interdiff.txt1.93 KBalexpott
#5 2413753.5.patch11.17 KBalexpott
#5 3-5-interdiff.txt6.89 KBalexpott
#3 2413753.3.patch4.28 KBalexpott
#1 2413753.1.patch4.47 KBalexpott

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2413753.1.patch4.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

dawehner
Primary language German
commented

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

I really like that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2413753.3.patch4.28 KB

Re-rolled now that #2368767: Implement calculateDependencies() in ArgumentPluginBase to get dependencies from validator and default plugins has landed.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs tests

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -Needs tests
StatusFileSize
new 3-5-interdiff.txt6.89 KB
new 2413753.5.patch11.17 KB

Added a display level test to ViewEntityDependenciesTest

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 5-6-interdiff.txt1.93 KB
new 2413753.6.patch11.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

dawehner
Primary language German
commented
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

olli commented
Status: Needs work » Needs review
StatusFileSize
new 2413753-9.patch11.02 KB
new interdiff.txt2.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

olli commented
Status: Needs work » Needs review
StatusFileSize
new 2413753-11.patch11.05 KB
new interdiff.txt1.22 KB
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

This is still RTBC IMHO.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This patch looks awesome!

catch’s picture

catch
he/him
Primary language English
commented
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.