Problem/Motivation

Base fields being indexed can create some interesting configuration:

  stores:
    label: 'Product » Product » Stores'
    datasource_id: 'entity:commerce_product_variation'
    property_path: 'product_id:entity:stores'
    type: integer
    dependencies:
      module:
        - commerce_product
        - commerce_product
        - commerce_product

Proposed resolution

Consider adding a small value object to make managing this simpler - that object could use \Drupal\Core\Entity\DependencyTrait

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3111635-8--fix_duplicate_dependencies.patch5.37 KBdrunken monkey
#8 3111635-8--fix_duplicate_dependencies--interdiff.txt3.91 KBdrunken monkey
#6 3111635-6.patch6.66 KBalexpott
#6 4-6-interdiff.txt629 bytesalexpott
#4 3111635-4.patch6.05 KBalexpott
#4 2-4-interdiff.txt1.15 KBalexpott
#2 3111635-2.patch4.9 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3111635-2.patch4.9 KB

This results in sorted and less duplicate dependencies in config.

Will need an update function to resave all of the Index config entities.

Status: Needs review » Needs work

The last submitted patch, 2: 3111635-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-4-interdiff.txt1.15 KB
new 3111635-4.patch6.05 KB

Ah I see the IndexImportTest is going to fail on 8.8.x and up because of changes for PHP 7.4. There's a simple fix.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Let's get this in. This change doesn't break anything and fixes forward compatibility.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 4-6-interdiff.txt629 bytes
new 3111635-6.patch6.66 KB

@borisson_ well the interdiff in #4 fixes compatibility.

In order to complete this patch we need an update path - fortunately doing this with core APIs is not too bad...

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This now has the compatibility fix that is high priority (interdiff in https://www.drupal.org/files/issues/2020-02-06/2-4-interdiff.txt).

The other changes look very good as well, I don't really have any remarks. We already had existing testcoverage for the change you did and it doesn't fail, so I think no additional testcoverage is needed.

And the actual code is in one of the more hairy parts of the system and it decreases complexity there, so that's very welcome. This also has an upgrade path to resave existing config items as well, so that is very nice. I wasn't aware that it was so easy to resave existing entities.

We should probably tell people that they need to re-export their config as well. But that is something we can add as to the release notes when the time comes to create a new one.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3111635-8--fix_duplicate_dependencies--interdiff.txt3.91 KB
new 3111635-8--fix_duplicate_dependencies.patch5.37 KB

Thanks a lot for reporting this issue and providing such a great patch! The Dependencies helper class is a great idea (pity it doesn’t exist in Core already – maybe some time in the future), and the post-update hook is also very educative.
All in all, this looks like a great little patch, I fully agree with Joris.
I just did a bit of coding standards clean-up, and also found a way to make the Dependencies class even smaller.

PS: Also great that Core managed to fix it’s PHP 7.4 fails. Now I can finally run all my tests locally again.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Back to rtbc, these changes look very good.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Great to hear, thanks for reviewing again!
Committed.
Thanks again, everyone, especially Alex!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/src/Utility/Dependencies.php
@@ -0,0 +1,26 @@
+  use DependencyTrait {
+    addDependency as public;
+    addDependencies as public;
+  }

Nice!

Status: Fixed » Closed (fixed)

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