Problem/Motivation

\Drupal\Core\Plugin\MapperInterface is a general-purpose interface to get plugin 'singletons' based on arbitrary 'options'. It is by no means a proper API, and it's only used by a handful of plugin managers.

Because the $options parameter is so generic, and different plugin managers behave differently based on the array's contents, the interface is useless by definition.

Proposed resolution

Remove the interface and transfer the method definition to the interfaces of the plugin managers that actually use it. In doing so, we can provide specific documentation for the $options parameter in the interface of every plugin manager.

Remaining tasks

None.

User interface changes

None.

API changes

\Drupal\Core\Plugin\MapperInterface will be removed. Contrib modules will, if they want to provide a similar feature, have to define a method with the same name in their own plugin managers' interfaces.

Files: 
CommentFileSizeAuthor
#48 drupal_1894130_48.patch22.98 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,625 pass(es).
[ View ]
#48 interdiff.txt612 bytesXano

Comments

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new5.2 KB
PASSED: [[SimpleTest]]: [MySQL] 50,699 pass(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, die-mapper-1894130-1.patch, failed testing.

msonnabaum’s picture

Status:Needs work» Needs review

Bot was just joking.

effulgentsia’s picture

Status:Needs review» Needs work

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.

effulgentsia’s picture

Category:bug» task
tim.plunkett’s picture

Category:task» bug
Status:Needs work» Needs review
Issue tags:+Needs tests, +Plugin system
StatusFileSize
new782 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dont-die-mapper-1894130-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

sun’s picture

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?

tim.plunkett’s picture

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().

sun’s picture

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?

tim.plunkett’s picture

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)

neclimdul’s picture

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.

sun’s picture

@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? :)

EclipseGc’s picture

@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

neclimdul’s picture

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."

effulgentsia’s picture

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 EntityManager 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.
msonnabaum’s picture

Ok, so we have a good explanation of what mappers are used for (what I would have guessed), but the question here is, why is it in the interface if its not always necessary?

If I have an instance of some PluginManager I see that it implements MapperInterface, that means that I can call getInstance() on it. The issue I see is, that's not always safe to do. PluginManagerBase defers to $this->mapper, but does nothing to ensure that exists.

To me, this just feels like functionality that can be tacked on for plugins that need it. Having it in the interface and the base manager is *really* confusing.

tim.plunkett’s picture

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.

msonnabaum’s picture

I agree that it will solve the issue in Drupal, but it won't solve the overall design issue.

effulgentsia’s picture

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?

msonnabaum’s picture

It still feels like a premature abstraction to me. Even the examples in #4 that depend on getInstance() are just doing the mapping in the plugin manager. I don't see why that's a bad thing. If we refactored those plugins to use Mappers, I have a feeling that someone would argue that the Mapper needs an instance of PluginManager anyways, so why bother with the middleman?

msonnabaum’s picture

To clarify, I agree with both points in #19, I just dont think its settled that MapperInterface is necessary.

sun’s picture

+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.

sdboyer’s picture

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.

EclipseGc’s picture

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

jibran’s picture

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

Status:Needs review» Needs work

The last submitted patch, 6: dont-die-mapper-1894130-6.patch, failed testing.

tim.plunkett’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new2.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch plugin-1894130-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

pwolanin’s picture

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.

pwolanin’s picture

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

anavarre queued 27: plugin-1894130-27.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 27: plugin-1894130-27.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new9.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal_1894130_32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Xano queued 32: drupal_1894130_32.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 32: drupal_1894130_32.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new9.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,351 pass(es).
[ View ]
pwolanin’s picture

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

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

Xano queued 35: drupal_1894130_35.patch for re-testing.

pwolanin’s picture

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.

Xano’s picture

Only PluginManagerInterface and PluginManagerBase directly use MapperInterface.

pwolanin’s picture

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.

Xano’s picture

StatusFileSize
new19.04 KB
new22.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,130 pass(es), 92 fail(s), and 0 exception(s).
[ View ]
Xano’s picture

Title:Consider removing Plugin\Mapper\MapperInterface» Remove \Drupal\Core\Plugin\Mapper\MapperInterface
Issue tags:-Plugin system
Xano’s picture

Issue summary:View changes

Status:Needs review» Needs work

The last submitted patch, 41: drupal_1894130_41.patch, failed testing.

pwolanin’s picture

Looks like some calls were missed:

Call to undefined method Drupal\rest\Plugin\Type\ResourcePluginManager::getInstance()
Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new574 bytes
new23.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es).
[ View ]
pwolanin’s picture

Status:Needs review» Needs work

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?

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new612 bytes
new22.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,625 pass(es).
[ View ]
tim.plunkett’s picture

  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?

Xano’s picture

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.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

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.

xjm’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs issue summary update, +Needs beta evaluation

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?