Having the property class implemented as plugins would allow us to use to do away with system_data_type_info() and use annotations instead.

But, do we want properties to depend on the plugin system?
Can the plugin system handle per-property overrides of classes, as well as doing two different plugins ('list class' + 'class') from the same definition?

Comments

fago’s picture

Title: Consider implementing properties as plugins » Should properties be plugins?
fago’s picture

As discussed in Munich, it would be nice to have data-types implemented as plugins. Right now we've an old-fashion info hook and system_data_type_info() for registering primitives. Being able to use annotations would be nice there!

>But, do we want properties to depend on the plugin system?
crell mentioned that he prefers not introducing the dependency on the plugin system, but that we should get a way from relying on the global drupal_wrap_data() function + that we could go with annotations without the plugin system.

I'm not so sure about that though. Having less dependencies is nice, yep - but without plugins we've to reimplemented annotations and the factory. If both works as it desired with the plugin system, I don't think we should we re-implemented the wheel for the sake of avoiding a dependency.

So let's try to figure out how we could leverage plugins:

For creating data type wrappers we currently have drupal_wrap_data() what takes a data definition array, a value and an optional context. The data 'type' in there would match or plugin_id, but we allow for per-property overrides of the plugin-class in the passed property definitions. So we'd need to figure out how we can do this kind of override. Value and context could be moved to separate setters, so I guess the property definitions has to be the passed plugin configuration.

Then we'd have:
plugin_id : data type name
plugin definition: current hook_data_type_info() items
plugin configuration: data/property definition

Then, which $owner and $type do we specify for those plugins? Possible 'TypedData' and 'type', thus we'd have directories ala Plugin\TypedData\type\. How do computed properties play into that? I guess those classes could just live somewhere else and be instantiated instead of the data type plugin by the factory.

Thus, we'd probably have a low-level plugin system factory for data types + a higher level factory function taking a data definition, value and context as drupal_wrap_data() now? E.g. TypedDataManager::wrapData($definition, $value, $context) ?

Lastly, how about performance? I've not seen any caching in the plugin annotations discovery method.

aspilicious’s picture

There is caching in the plugin system by using a cacheDecorator around the discovery. It *doesn't* support languages yet and thats a critical bug/task at the moment.

Annotations should only be used for discovery. In the initial views to annotation port we put a lot of crap inside annotations. But this means the cached definition arrays contain more than just discovery code.

So in views we decided to just leave the standard discovery items in annotations. (all the stuff you can find in the aggregator annotation). All the other info is packed inside the class as properties.

Cached annotations are working prety fast in views now.

fago’s picture

fago’s picture

Thinking more about it, the 'list class' and 'class' entries have to be separate plugins - thus we need to have separate plugin definitions / data types for them. For what we already have: #1762874: Decouple property item lists from property items.

Sylvain Lecoy’s picture

I personally don't think it would be a good idea.

What would be the use case for this ? If you want to provide swappable data type, such as date for instance, change the entity definition seems wiser than provide a plugin layer for properties.

I think it will introduce overhead.

fago’s picture

The plugin system is about providing plugins of a certain type based upon a defined interface, i.e. data types, so it's not necessarily about swapping out something.
But then, once you have plugins you can choose with which of the plugins to work with - what corresponds to changing the entity definition.

Sylvain Lecoy’s picture

This is something above my knowledge right now to provide a good opinion.

What does system_data_type_info(), and why it is needed is for me obscure but I believe having the plugin system required for the properties looks not appealing. A part from throwing away system_data_type_info(), what are (for a non entity expert programmer point of view) the benefits ?

fago’s picture

The benefits I see are:

  • Less code and improved code reuse as we could leverage the plugin system's discovery method and factory.
  • Annotation discovery, making discovery independent from the module system and bringing the class and its metadata together.
fago’s picture

As of now we really have two kind of classes:

  • Typed data wrappers (=the class that's used for wrapping typed data by default)
  • Custom wrappers (= customly specified classes that can be used in certain situations). This is used to implement a computed property - just specify your own wrapper that takes care of loading.

Not sure how custom wrapper classes would work with the plugin system best. We could just leave them as custom classes as they are now and incorporate them in a custom factory.

Sylvain Lecoy’s picture

Metadata should be possible on entity properties without the need for properties to be plugins isn't it ?

Actually, I never seen a similar approach so that's why i'm a bit surprised. How this would impact large entity set performance for loading and rendering operations?

yched’s picture

AFAIK, a dependecy on the plugins system just means a dependency on :
- a PSR-0 loader
- the cache layer for CacheDecorator
- using annotations, you don't even strictly need the modules system, unlike with an info hook
So I don't think I see what big of a deal that would be.

Performance-wise, the main impact we found in #1785256: Widgets as Plugins was class autoloading - and we already know our autoloader has perf issues.
But our issue is about moving away from callbacks to methods in plugin classes, so we're loading more classes than before; here your datatypes are already classes, the only additional classes in you case would be your custom DatatypePluginManager and your Factory - the discovery classes will probably be loaded by other plugin types in the request anyway.

effulgentsia’s picture

So, I definitely think we should change drupal_wrap_data() to a proper Factory and have a Manager interface for fetching data type info instead of hard-coding a reliance on an info hook. This doesn't necessarily require us using the Plugin system, but the Plugin system already does these things, so I think it makes sense to start off using it, and then open a follow up issue for implementing a custom factory and manager that doesn't rely on the Plugin system if there's a compelling reason to remove the dependency of Property API on Plugin API.

fago’s picture

#13: Agreed.

For using annotations I think we'd need to look into modifying the AnnotationReader so it parses plugins from a non-module location.

fago’s picture

Title: Should properties be plugins? » Implement data types as plugins.
Status: Active » Needs work

ok, I've done so and converted data types to plugins. For now I've just leveraged the hook discovery, but migrating to annotations can now be done easily thanks to the plugin system.

For that I've created a 'typed_data' service which allows for easy access to the typed data manager. Then, I've added a create() method to the manager which wraps the usual plugin system factory and has the same function signature as drupal_wrap_data() before. Also, there is shortcut for getting the typed data service: typed_data(), similar to language() and state().

Thus, a typed data object is now created like that:
$data = typed_data()->create($definition, $value);

Note, that this fixes:
#1753452: Remove dependency on drupal_wrap_data()
#1753472: Remove the constructor from data wrapper interfaces

Setting to "needs work" for annotations.

fago’s picture

ok, discussed with eclipsegc on IRC:

For using annotations I think we'd need to look into modifying the AnnotationReader so it parses plugins from a non-module location.

Either this or we've a stand-alone component with its own register namespace anyway. As we've not we figured out it's best to adapt the AnnotationReader such that one can pass in additional namespaces during construction.

Given that we could do the following plugin locations picked up by the annotation reader:

\Drupal\Core\TypedData\Plugin\Core\TypedData\*
\Drupal\Core\Entity\Plugin\Core\TypedData\*
\Drupal\module\Plugin\Core\TypedData\*

eclipseGc also liked having a separate plugin directory for all of core, e.g. \Drupal\Core\Plugin\Core\TypedData\*. Somehow that looks neat to me as well, but I'm worried that the plugins are not associated with a subsystem anymore - so typed data entity plugins and plugins provided by the typed data system itself would be mixed up. Then, e.g. - where do you look for a plugins test case then?

Next, I must say that for Entity API DX having TypedData plugins is perhaps a DX--. E.g. I'd prefer having module\Plugin\Entity\Property\* there. So maybe we should just postpone this for now and continue the discussion as a follow-up? Thoughts?

effulgentsia’s picture

Status: Needs work » Fixed

I think it makes sense to consider this issue fixed, and to open a new core issue for annotations after the initial Property API patch lands.

dixon_’s picture

I agree with @effulgentsia, let's leave it with what we have now and move forward with other things. Improving the annotation reader and making use of it can be done in a post code freeze follow-up I believe.

fago’s picture

Yes, let's do it that way. I'd like to explore the idea of having entity property plugins that extend typed data plugins so it appears in a logical way to devs, so let's focus on this in a follow-up.

Status: Fixed » Closed (fixed)

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