Use annotation discovery for data type plugins

What are you going to name your annotation class?

Note: I wrote the patch based on #1969728: Implement Field API "field types" as TypedData Plugins as suggested by @fago. Please review 1867856-including-field-type-plugins.patch

#6: 1867856-including-field-type-plugins.patch queued for re-testing.

#1969728: Implement Field API "field types" as TypedData Plugins just got commited. Here is a re-roll including fixes for those test fails.

Same patch, just with moves, so that the patch is much easier to review.

+++ b/core/lib/Drupal/Core/TypedData/Annotation/DataType.phpundefined
@@ -0,0 +1,35 @@
+/**
+ * Defines a data type annotation object.
+ *
+ * @Annotation
+ */
+class DataType extends Plugin {
+
+  /**
+   * The name of the module providing the type.
+   *
+   * @var string
+   */
+  public $module;
+
+  /**
+   * The name of the data type class.
+   *
+   * This is not provided manually, it will be added by the discovery mechanism.
+   *
+   * @var string
+   */
+  public $class;
+
+}

We probably want to put more in there (like constraints, etc.). The rest is mostly moving things around.

+++ b/core/lib/Drupal/Core/TypedData/Annotation/DataType.phpundefined
@@ -0,0 +1,35 @@
+class DataType extends Plugin {

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Any.phpundefined
@@ -15,6 +17,11 @@
+ * @DataType(
...
+ *   label = @Translation("Any data")
+ * )

The point is to document each property, so please add label and maybe also description.

This issue just moves stuff around. That documentation was not introduced here. Lets fix it in a follow-up.

Ok, I reviewed this and then managed to lose my review so, me--

The short version is this:

The Annotation class is all wrong, none of the stuff in it needs to be there and the stuff that should be there isn't.

The TypedDataManager should extend DefaultPluginManager and leverage the goodness there. The TypedDataManager should also stop proxying its stuff to TypedDataFactory. We should just move the contents of TypedDataFactory into the manager and call it good. Likewise all references to $this->discovery and $this->factory should go once that cleanup is done.

It should all look a little something like this:

Eclipse

After a little discussion with fago in irc I found that there should be an alter hook for the definitions, so I readded that back in. if 18 comes back green, then we're obviously missing tests. This patch also fixes his description issue from 20

Eclipse

Just wanted to note that, should this issue does not reach resolution, hook_data_type_info() would need to be documented: it current has two implementations, but no definition in a .api.php file.

@fgm: This patch removes a hook_data_type_info() in system.api.php?

@berdir : yes, I can see that now. Don't know why I could not find it yesternight, must have been too late. Just ignore my remark.

Refactoring TypedConfigManager to include SchemaDiscovery and that factory and extended PluginManagerBase directly instead.

Ok, that whole manager is totally weird, but if we have to hack around it for the time being, I'm 100% ++ My contributions to this were specifically around using DefaultPluginManager which is pretty straight forward and well represented elsewhere in core, so I feel pretty comfortable rtbcing this.

Eclipse

Though I don't really understand the full extent of this patch, the TypedConfigManager changes look ok to me

Ok then, let's try this again.

Eclipse

Related, as we are moving around SchemaDiscovery, there's some issue with that class: it loads all the data from the constructor instead of from getDefinitions(), and that's the reason why just instantiating it adds some processing and creates lots of plugins...
(That could be another patch I guess, just in case this needs some reroll it could be fixed here too, otherwise I'll create some follow up...)

That entire PluginType (TypedConfigManager) needs some serious review. I think that needs its own issue though.

Eclipse

Fixing some problems with TypedConfigManager. Jose found a performance problem with TypedConfigManager which we can fix right here. The schema should not always be loaded on construct directly but rather only when it's requested in ::getDefinitions(). Also, ::getDefinition() should not alter $this->definitions. In fact, there should probably be another method that covers these fallback scenariors. But that's follow-up material.

Okay, RTBC for #29 again. Let's fix the performance issues in a follow-up so we can get this in now.

Please commit the patch from comment #29

Opened a follow-up for the TypedConfigManager and uploaded the patch there: #2030859: Improve TypedConfigManager. Let's get #29 in asap (very helpful for param converter / typed data).

*meow*

I blame the reviewers.

Assuming that passes, this is still rtbc. It was just a comment change.

Eclipse

Raising to major, as discussed with @xjm.

Committed 2c2f5f5 and pushed to 8.x. Thanks!

There's some left - follow-up #1913620: Move comment TypeData to proper place

Updated https://drupal.org/node/1806650 and added an example of how to define a data type, did not exist before.