Make \Drupal\Core\Plugin\Mapper\MapperInterface optional for plugin managers

In order to do this, we'd have to do #1889748: Figure out what to do with ConfigMapper.

There are many plugin types for which getInstance() never needs to be called, so that's one point in favor of removing MapperInterface from something that is required of PluginManagerInterface.

However, there are some plugin types for which getInstance() is an important part of the manager's interface. Current examples in HEAD include FormatterPluginManager and WidgetPluginManager, but that list will likely grow, both in core, and in contrib.

So at a minimum, we should retain MapperInterface, and allow managers that want to implement it to do so.

Also, PHP only allows type hinting to a single type at a time. So you can't type hint to PluginManagerInterface and MapperInterface at the same time. I don't know if that warrants us creating a MappablePluginManagerInterface in order to allow such a type hint. It's possible there's not much use case for that, since something that wants a FormaterPluginManager can type hint to that, so the question is whether there's a use case for type hinting to MappablePluginManagerInterface generically.

This is a bug, because if you call getInstance(), a public method, on most managers, it will fatal.

We should have a test case to just call getInstance() on a manager with no mapper, and assertFalse().

Also, I'm fixing the docblock here because it's what the rest of core does, and if you remove the method, the error is Fatal error: Class Drupal\Core\Entity\EntityManager contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Component\Plugin\Mapper\MapperInterface::getInstance) in /Users/tim/www/vdc/core/lib/Drupal/Core/Entity/EntityManager.php on line 130, and it doesn't mention PluginManagerBase.

Since the only implementation right now is ConfigMapper, I suspect that the original intention of "mappers" was to achieve a "swappable plugin configuration storage"?

I.e., mappers are to the plugin system like storage controllers are to the entity system?

If so, that's probably a good thing to retain. Especially since this lives in \Component\Plugin.

However, a blatant s/mapper/storage/g would be helpful, and of course, removing the baked-in default call from PluginManagerBase would be sane, too.

That still leaves me with the question of where $this->mapper gets actually populated/instantiated though. Given the current architecture, I'd naturally assume in one the constructors of the plugin manager hierarchy, but that doesn't seem to be the case?

In general, the mapper/storage looks more like something that would be injected into the manager, instead of being constructed by the manager?

It only ever existed in BlockManager, before the switch to ConfigEntity: http://drupalcode.org/project/drupal.git/blob/6e7b121:/core/modules/bloc...
But it was definitely intended to be in PluginManagetInterface::__construct().

Yeah, and it appears to me that the code of ConfigMapper is a super-lightweight incarnation of ConfigStorageController.

Instead of going through the entity system, it

1. uses config()
2. loads the raw config object
3. extracts its 'id' config key as $plugin_id
4. puts all other contained config keys into $settings
5. tacks the passed-in $options['config'] onto $settings['config_id']
6. and lastly creates a new plugin instance via $this->manager->createInstance($plugin_id, $settings);

Within Drupal, I almost guess it's safe to say that this entire thing is obsolete and unwanted.

Instead, for config-backed plugin instances in Drupal, we want this:

1. Use entity type X with ConfigStorageController
2. Load the config entity.
3. Extract the ID from the 'plugin[_id]' property into $plugin_id.
4. Uses $entity->settings as $settings.
5. Uses $entity->id() as $config_id (that's a misnomer: $config_name, plz).
6. Creates a plugin instance via $this->manager->getInstance($plugin_id, $entity);

Or not? What do you think?

Not 100% sure what you mean by #9.4 and #9.5, but the rest look correct. #9.6 alternately should use PluginBag, when an entity can have more than one plugin that isn't always used (like Views displays).

We also haven't talked about if its too much overhead, but we might consider always using it when an object has 2-N plugins, even if it doesn't need them lazily instantiated. (Formats->filters, Image styles->image effects)

I would like to draw attention to "Within Drupal, I almost guess it's safe to say that this entire thing is obsolete and unwanted." emphasis mine obviously. Plugins where actually designed to be generically useful and not tied to Drupal which is why they exist in Component. We should be careful of saying "yeah Drupal doesn't need that anymore because we can use entities" when it hamstrings the system on its own.

@neclimdul: Sure thing. However, as one of the original authors of the plugin system, any chance you could shed some light on the more substantial question of whether $mapper was essentially supposed to mean $storage? :)

@sun, I can't answer on neclimdul's behalf, but for my own part...

Mapper is literally mapping something to a plugin instance. What that something is can be totally arbitrary so long as you can provide the logic in order to "map" to a plugin instance (which is why it's such an obtuse "array', and not something more specific like a string or a particular interface). I've seen you observe that getInstance ultimately calls create instance, this is because in the 99% case, we are literally getting a new instance and injecting values into it, so while your observation of "storage" may be valid in certain circumstances, on the whole we picked the term "mapper" because we thought of it in terms of mapping some thing to a set of values that could help us create a fully configured plugin instance. Obviously, this flow is not a requirement, but that's the basic logic (as I recall it).

Eclipse

what eclipse said. Storage is relevant but not the only option. It could be some sort of de-serialization logic, runtime logic, etc. At design it was envisioning possibly wrapping fallback logic as well like "this plugin doesn't have requirements in place like a database connection so here's a fallback implementation which will meet your needs for now."

Adding a little to #13 and #14.

Basically, FactoryInterface::createInstance() is what consuming code calls when it knows the $plugin_id and $configuration it wants to instantiate. MapperInterface::getInstance() is what consuming code calls when it knows some other information ($options) and wants the plugin manager to decide what the correct $plugin_id and $configuration is for those $options.

Examples:

1. Cache: $plugin_id might be 'memcache' or 'database'. $configuration might include 'host', 'port', 'username', 'password', 'bin'. Consuming code might only know 'bin', and calls MapperInterface::getInstance(array('bin' => $bin)) to let the plugin manager figure out the correct plugin id (backend) and its complete configuration to use. Note that we have not yet converted the cache system to the plugin API, and I don't know if we will for D8.
2. Blocks (prior to ConfigEntity conversion): $plugin_id might be 'user_online_block' and $configuration might include 'seconds_online', but the calling code just knows $machine_name, which is something administrator entered and not the same as the $plugin_id. So the calling code calls MapperInterface::getInstance() to let the block manager figure it out, which it does via the ConfigMapper loading the CMI file and getting the info from there. Post ConfigEntity conversion, this no longer applies, because the config entity now does this work, so can all createInstance() directly on the block manager.
3. Entities: Not yet fully settled in #1867228: Make EntityTypeManager provide an entity factory, but as I've argued for in that issue, an entity_load() is a situation where you know the entity id, but do not know all the values you need to set on the instantiated $entity. To get those values, you need to query the storage backend and invoke the load() hooks. So to me, that maps pretty clearly to the MapperInterface::getInstance() concept.
4. Field API formatters and widgets: the consuming code actually pretty much knows $plugin_id and $configuration, except there's the possibility that the plugin id (formatter type, widget type) it wants to instantiate isn't available, and therefore, a different (default) plugin id needs to be instantiated instead. That kind of fallback is intentionally not part of the scope of FactoryInterface, so MapperInterface::getInstance() is used to implement it.

I agree, but I think if we remove it by default, no one will ever find it.
That said, #6 will half-heartedly address #16.

So I think the two questions are:

- Do we want it on PluginManagerInterface? #16 is a good reason for no, and if that's a good enough reason to remove it, how do we address #4?
- Even if we want to remove it from PluginManagerInterface, do we still want an implementation on PluginManagerBase? I think that would be useful only if we actually intend to have some generically useful class that could be instantiated into $this->mapper. Otherwise, it might be the case that any useful implementation of getInstance() needs to be manager-specific anyway, in which case, no need for the base class to implement it at all.

But in any case, seems like there's agreement on keeping MapperInterface itself around, yes?

+1 to whatever we can do to simplify.

I studied the plugin system code a bit more last night for #1901380: [Block]PluginManager repetitively triggers full-stack Annotation parsing on a plugin instance ID miss and by now I'm certain that the prematurely optimized and over-abstracted parts of the plugin system are the cause for performance problems. While those are just caused by logic problems (and those can be fixed), it's very hard to follow the code execution paths, and in general, too much "base" logic is left to individual implementation classes, which in turn means that logic/performance bugs can be introduced in plenty of classes scattered across the system.

The relation I see to this issue is that MapperInterface is an abstraction that's unused currently, and thus, also a source for potential bugs. Especially since there is a force-exposed method, but which, apparently, is not supposed to be used.

I'd recommend to remove MapperInterface. At least from PluginManagerBase.

We can keep the ConfigMapper and ReflectionMapper around. We could also leave the MapperInterface around unused. However, I additionally think that the $options array signature of getInstance() is not really helpful, and rather poor DX. To make the mappers useful in the long-term, I think that MapperInterface should consist of more specialized methods.

If a PluginManager implementation needs them, it can always do so.

i'm in favor of removal.

architecturally speaking, the problem (for Drupal or others) is that MapperInterface::getInstance() is a faux interface. its signature calls for an anonymous array, which means instead of presenting a clear contract for consumption (which is what interfaces are for), it muddles everything by requiring that there be specially named keys on the array which particular implementations of the interface know how to use. that makes it, by definition, not an interface, because it defines no common, established form of communication for either consumer or implementor. this is demonstrated perfectly well by ConfigMapper, which relies on a specially named key in the parameter, 'config'.

in working on the SCOTCH princess branch, i ended up writing a slightly different interface, which i've dumped in a gist:

https://gist.github.com/sdboyer/5312082#file-configdrivenmapperinterface...

feel free to ignore the other things in there (though they are intended as part of a more complete whole). that interface could be appropriate for any plugin class whose configuration is backed by config entities. it makes sense as a "mapper" because it says, specifically, what parameters it takes, and explains how they'll be used to produce a plugin instance, with its configuration injected.

truth be told, while we have a specific notion of a "configuration id", for the same reasons that it wasn't unreasonable to attempt a ConfigMapper in Drupal\Component in the first place, the interface in that gist could be put in Component, as well.

So, I think the important take away here is that we generally acknowledge that we have a use for such a concept, but for the same reasons I pushed for the interface to be just a simple array passed (we can't know what parameters might be necessary to "get" and instance) you're saying "remove the interface completely and just create smaller less generic interfaces for the specific use cases of "getting" an instance your plugin type might have."

In principle I'm actually on board with the approach and I _THINK_ it'll be pretty easy to implement at this point since we don't have many classes that implement this interface currently. That sort of unformalizes the approach, but the ability to type hint parameter for your use case w/o a bunch of manual validation of what came across the options array seems like a win to me. Ultimately I think I'm ++ here, but I'll just point out that reusable "mappers" are going to be few and far between, and most plugin types should just write what they need from this perspective directly into some method of the manager.

Eclipse

6: dont-die-mapper-1894130-6.patch queued for re-testing.

It's now being used by:

• ArchiverManager
• FormatterPluginManager
• WidgetPluginManager
• MailManager
• TypedDataManager
• SelectionPluginManager
• ResourcePluginManager

Here's an alternate approach from the dupe #2246805: Prevent fatal errors when calling getInstance on plugin managers without mappers I opened.

We could remove it from ResourcePluginManager as redundant - every use there could trivially be replaced with createInstance()

Throwing an exception seems a bit strange when the method is in the interface, and I'd also agree with sdboyer than somehow creating a plugin from an anonymous array is dubious DX.

For managers that need to do this kind of mapping, it would seem they could extend the interface to add a method with specific arguments as suggested. Then we'd have a specific type hint and git rid of the generic Mapper inteface.

Is there agreement with moving to specific mapper methods or some reason to keep this generic interface?

What about this? We remove MapperInterface from PluginManagerInterface, but we keep it on all the plugin managers that currently use it by adding it to their interfaces. This should not introduce any BC breaks, yet makes PluginManagerInterface much leaner.

Hmm, I hadn't realized that interfaces (unlike classes) supported multiple inheritance.

This seems fine as a way forward to simplify the common interface.

Is anything type-hinting on the mapper interface? Might almost make sense to just added the 1 method to the 3 interfaces so you could have more through documentation or diverge them if needed.

Only PluginManagerInterface and PluginManagerBase directly use MapperInterface.

So, to clarify - the suggestion is to move the getInstance() method to the 3 new interfaces with more docs about what options keys/values are supported and rm the MapperInterface which is so generic as to be unhelpful.

Chatting with Xano on IRC - he may re-roll with this change, or if not I might do it later.

Looks like some calls were missed:

Call to undefined method Drupal\rest\Plugin\Type\ResourcePluginManager::getInstance()

Minor, in doxygen for MailManagerInterface

+   * Overrides PluginManagerBase::getInstance().

Also, seems like we no longer need a specific ResourcePluginManagerInterface, though I guess that's helpful for typehinting?

1. +++ b/core/modules/rest/src/Plugin/Type/ResourcePluginManager.php
   @@ -39,12 +39,4 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   -  public function getInstance(array $options){
   -    if (isset($options['id'])) {
   -      return $this->createInstance($options['id']);
   -    }
   -  }
   

   Hilarious.

2. +++ b/core/modules/rest/src/Plugin/Type/ResourcePluginManagerInterface.php
   @@ -0,0 +1,21 @@
   + * Definition of Drupal\rest\Plugin\Type\ResourcePluginManagerInterface.
   

   Should be Contains now. But why do we even have this without any methods? Just for future use?

But why do we even have this without any methods? Just for future use?

It can be useful for documentation and type hinting if we choose to extend it in the future. I don't see a real problem in removing it again, though.

since we're messing around with rest resources in other issues, an interface & swappability may be relevant in the future. It's ok empty for now.

The title and the summary of this issue do not seem to be up to date... since the patch does not remove MapperInterface nor even deprecate it. Also, since this seems like it includes API changes, we need a beta evaluation that documents the positive impact of this patch and the disruption introduced by it. Finally, has @tim.plunkett's feedback been addressed?

@tim.plunkett - yes, the specific interface is for use in type hinting. Seems better to define it now rather than potentially have people doing a generic type hinting?

@EclipseGc also pointed out ArchiverManager has a ::getInstance() method we forgot to convert.

+++ b/core/modules/rest/src/Plugin/Type/ResourcePluginManager.php
@@ -39,12 +39,4 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
-  /**
-   * Overrides Drupal\Component\Plugin\PluginManagerBase::getInstance().
-   */
-  public function getInstance(array $options){
-    if (isset($options['id'])) {
-      return $this->createInstance($options['id']);
-    }
-  }

+++ b/core/modules/rest/src/RequestHandler.php
@@ -42,7 +42,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
-      ->getInstance(array('id' => $plugin));
+      ->createInstance($plugin);

WTF? This should never have been setup this way. Regardless of how this issue pans out, this needs to change. I'd suggest making a separate issue for this right now. Seems like an easy win.

So... the TL:DR on this patch is "remove the one generic Plugin interface for 'mapping' and replace it with a bunch of interfaces that are identical for the specific situations in which a manager needs to do mapping". I'm not sure I consider this a win. In previous comments I ++'d this idea, but that was caveated with the point that we'd be type-hinting our needs. Thus far we've not done that at all, and worse still this patch doesn't even hit all the Mappers. Off the top of my head, ArchiverManager isn't in this patch, and that's pretty much the canonical use of Mappers in core. It makes me leery of this patch that it's missing a 2+ year old change to core. This is totally NW.

Eclipse

I'm not sure what you mean by type-hinting in this particular case, but changing method signatures for the managers that really do use ::getInstance() would be even more of a BC break. At the very least the current approach allows us to add much more specific documentation to managers.

Discussed with @pwolanin and @EclipseGc on IRC that we'll just take the original approach of keeping MapperInterface for the plugin managers that use it, but remove it from PluginManagerInterface.

This is a re-roll of #35.

WTF? This should never have been setup this way. Regardless of how this issue pans out, this needs to change. I'd suggest making a separate issue for this right now. Seems like an easy win.

#2503355: Remove ResourcePluginManager::getInstance().

The entire idea was to remove the interface because it was unnecessary.

If the interface is unnecessary, making its usage optional seems like a step in the right direction. Admittedly, it's not a total fix.

Also, the issue summary changes seem pretty non-sensical to me. I don't really get the direction this has taken.

The issue summary still describes the patch in #53, from which we have since departed by merely making the interface optional.

Several people have expressed their concerns about the two different solutions that have been proposed. In addition to the feedback, can everybody also share their ideas about how this issue should be fixed?

Can we get consensus on this, please? We've tried multiple ideas that were all shot down at some point. This is a real bug that on top of breaking things also confuses people; I have talked to several trainers who don't even include mapping when they explain the plugin system.

At the very least, MapperInterface should become optional. This is an easy fix on top of which we can continue improving later.

I agree, making it optional is better than nothing.

Is this good to RTBC, then? I'll happily make a follow-up to remove the interface altogether.

@EclipseGc made a general +1 on this issue so yeah let's simplify things.

Re-testing, because I cannot reproduce the failures locally.

Back to RTBC per #69 and #72.

The consensus seems to be that doing this is worth the disruption but this is definitely disruptive since all contrib plugin managers need to at least consider the impact of the change. Therefore we need a change record which tells developers what to check for and what to do.

PluginManagerBase still provides an implementation of this method (but does not actually implement MapperInterface). Should we do anything about that?

#76 sounds like a question worth considering and not much like an rtbc.

I removed PluginManagerBase::mapper and ::getInstance(). The method was never called, as all calls were made to the method's overrides in child classes, and the property was only used by the method. As such, there is nothing in core that needs them anymore.

We should probably update the CR to include that.

The CR now includes this.

Random test fail.

We said we'd only break API's to solve criticals in 8.0.x. I can't see what advantage the current change gives us over a proper fix. Both break APIs. I can't see how this is eligible for the beta. Given I'm not a release manager I'm going to assign for release manager to make the final call - since there are people who think this change is worth the disruption.

For #85.

Do you have an alternative solution to the currently broken interface implementations in mind?

@Xano, can you clarify what is "broken"? All I see on the issue and in the summary is that the interface is "useless" and that it is not good DX, which is not sufficient reason to break APIs at this point.

You're absolutely right. Thanks for pointing this out.

PluginManagerBase::getInstance() uses a mapper object that does not exist, causing a fatal error when called. Only a handful of core's plugin managers actually have a working implementation of this method, and they all override the parent. This means that PluginManagerBase::getInstance() is not only broken, it is also never used in core.

The usual argument in this case is that even if something is broken or stupid, if it doesn't cause direct problems, we just deprecate it and postpone removal to Drupal 9. In this particular case, as demonstrated by PluginManagerBase::getInstance(), the interface forces plugin managers to provide a feature (plugin mapping) that most of them can never provide. This way the system pretends to support plugin mapping, while actually it's horribly broken in most places.

Our POV was that clarity and a system that's not fundamentally flawed warranted the disruption. Then again, we're on RC's doorstep right now, which might change things.

We could deprecate the method on PluginManagerBase in 8.x and trigger E_USER_DEPRECATED if anything calls it.

We can also have the classes that implement mapperinterface do that explicitly themselves in 8.x

It also looks like the rest plugin manager could have an interface added if it needs one.

Then the 9.x change would only be removing the deprecated methods/interface implementation from the base class.