Problem/Motivation

\Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies():

Calculates and adds dependencies of a specific plugin instance.

However, in a case where you need to compare those dependencies to another list (like in onDependencyRemoval()), the adding of the dependencies is not desirable.

Proposed resolution

Move the logic of calculatePluginDependencies() to a protected method called getPluginDependencies(), and use that within the calculate method.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#24 2939925-plugin-24.patch7.04 KBtim.plunkett
#14 2939925-plugin-14-interdiff.txt1.76 KBtim.plunkett
#14 2939925-plugin-14.patch7.04 KBtim.plunkett
#10 2939925-plugin_dependencies-7.patch5.27 KBjibran
#9 2939925-8.patch2.63 KBjibran
#5 plugin_system-trait-2939925-D8.patch2.39 KBioana apetri

Comments

tim.plunkett created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Novice
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes

Clarifying that the method should also be protected

ioana apetri’s picture

ioana apetri commented
Assigned: Unassigned » ioana apetri
ioana apetri’s picture

ioana apetri commented
Assigned: ioana apetri » Unassigned
Status: Active » Needs review
StatusFileSize
new plugin_system-trait-2939925-D8.patch2.39 KB

Here are the modifications, Please review.
Thanks:)

Status: Needs review » Needs work

The last submitted patch, 5: plugin_system-trait-2939925-D8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
@@ -27,26 +27,37 @@
+      $dependency = $this->addDependency('module', $definition->getProvider());

This new method must not alter the state of the object. So it cannot use addDependencies.

I think the method needs to start with a $dependencies variable, and then use \Drupal\Component\Utility\NestedArray::mergeDeep() to make subsequent additions. And then return it at the end.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 2939925-plugin_dependencies-7.patch5.27 KB

This is what I was describing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new 2939925-8.patch2.63 KB

I think the method needs to start with a $dependencies variable, and then use \Drupal\Component\Utility\NestedArray::mergeDeep() to make subsequent additions. And then return it at the end.

Done.
No interdiff because I started from scratch.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2939925-plugin_dependencies-7.patch5.27 KB

X-post :/. Please ignore my patch. Re-uploading #8.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we need a test that the new getPluginDependencies() method does not alter the state of the object. And I wonder if we should have any BC concerns with adding a method to this trait.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

If a class uses that trait and already has a getPluginDependencies(), the new addition to the trait will be ignored and the original method will still be used.

Any recommendations on testing the object state? Off the top of my head I can only think of using Reflection.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Would it work if the test used the trait like a normal class and then the test would have the property from the trait to test against?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
Issue tags: -Novice, -Needs tests
StatusFileSize
new 2939925-plugin-14.patch7.04 KB
new 2939925-plugin-14-interdiff.txt1.76 KB

Added a test.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Thanks, test looks good.

eclipsegc’s picture

eclipsegc commented
Status: Needs review » Reviewed & tested by the community

This totally empowers the plugin dependency calculation much better than current and removes a ton of custom code that shouldn't be specific to LayoutBuilder so, I'd say RTBC on this. Tests look good, patch looks good.

Eclipse

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Can we use this in Workflows module too?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed

This issue is a step on the way to fixing #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed. Both LB and Workflows have @todo's pointing to that issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3988196 and pushed to 8.6.x. Thanks!

  • alexpott committed 3988196 on 8.6.x 
    Issue #2939925 by jibran, tim.plunkett, yo30: Add a...

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Closed (fixed) » Reviewed & tested by the community

This blocks #2943627: Layout Builder does not consult layout plugin for dependencies going in to 8.5.x

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2939925-plugin-14.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2939925-plugin-24.patch7.04 KB

Not sure why it didn't apply, but this patch was generated by cherry-picking the 8.6.x commit.

Also, in case this does not make it into 8.5.x, I updated the blocked issue to work around it...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 8.5.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Per #2943627-43: Layout Builder does not consult layout plugin for dependencies, not going to backport this one.

Status: Fixed » Closed (fixed)

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

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented

There is a line of code added in the patch here that is needed in Drupal 8.5, because NestedArray is present in Drupal 8.5.x PluginDependencyTrait.php but is missing a use namespace. (Discovered while testing a backport of #2904550: PluginDependencyTrait::calculatePluginDependencies() does not handle theme provided plugins).

Ignore me. Appologies, the NestedArray does NOT exist in D8.5.x without the patch on the ticket I was backporting.