Spin-off from #1867856: Use annotation discovery for data type plugins. We found that the SchemaDiscovery (which we removed in the aforementioned issue and merged it into the TypedConfigManager) should not run the discovery mechanisms in the constructor but instead just invoke that in getDefinitions() when requested. Patch incoming (including some other minor improvements).

CommentFileSizeAuthor
#9 interdiff.txt577 bytesfubhy
#9 2030859-9.patch5.16 KBfubhy
#7 2030859-7.patch5.4 KBfubhy
#4 2030859-4.patch5.33 KBfubhy
#1 2030859-1-for-review.patch5.33 KBfubhy
#1 2030859-1-for-testbot.patch78.93 KBfubhy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Status: Active » Needs review
FileSize
2030859-1-for-testbot.patch78.93 KB
2030859-1-for-review.patch5.33 KB
Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
Status: Needs review » Needs work

Agree with the need to fix SchemaDiscovery, but this patch looks a bit too long to me...? (It looks like it includes too #1867856: Use annotation discovery for data type plugins )

fubhy’s picture

fubhy CreditAttribution: fubhy commented

Yes, @see the "for-review" patch ;).

fubhy’s picture

fubhy CreditAttribution: fubhy commented
Status: Needs work » Needs review
FileSize
2030859-4.patch5.33 KB

Re-uploading that patch for testbot :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +D8MI, +language-config

Looks good to me.

fubhy’s picture

fubhy CreditAttribution: fubhy commented 
+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.phpundefined
@@ -104,11 +104,11 @@ public function create(array $definition, $value = NULL, $name = NULL, $parent =
+      throw new \InvalidArgumentException(format_string('Invalid data type %plugin_id has been given.', array('%plugin_id' => $plugin_id)));

Actually, that should be String::format() :(

fubhy’s picture

fubhy CreditAttribution: fubhy commented
FileSize
2030859-7.patch5.4 KB
Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Yes, this one looks good to me, only one minor issue:

       unset($definition['type']);
-      $this->definitions[$type] = $definition;

Could we keep this last line so we don't need to reprocess the same definition twice and it gets kind of cached, mostly for performance reasons?

fubhy’s picture

fubhy CreditAttribution: fubhy commented
FileSize
2030859-9.patch5.16 KB
interdiff.txt577 bytes

Sure... Dunno how this affects the tests or why I even removed it. But we'll see! :)

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
Status: Needs review » Reviewed & tested by the community

Looks good and green :-), thanks, I think this is ready.
(and required for this one to work: #1966538: Translation is not updated for configuration, when the config changes)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +blocker

Tag as blocker being a dependency.

alexpott’s picture

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

Committed d7bbf9f and pushed to 8.x. Thanks!

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