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.

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.

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.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because nothing is broken, but the impossibility to standardize/document the interface makes it useless and decreases DX
Issue priority Normal
Prioritized changes The main goal of this issue is DX and documentation improvements.
Disruption Possibly disruptive for contributed modules. If any code uses MapperInterface, modules can cope with this change by copying MapperInterface's old method to their own interfaces. This also allows them to improve their own documentation.

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?

Xano’s picture

Title:Remove \Drupal\Core\Plugin\Mapper\MapperInterface» Remove \Drupal\Core\Plugin\Mapper\MapperInterface and merge its functionality into the few interfaces that need it
Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs issue summary update, -Needs beta evaluation
StatusFileSize
new1.78 KB
new24.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal_1894130_53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 53: drupal_1894130_53.patch, failed testing.

Xano’s picture

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

EclipseGc’s picture

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

Xano’s picture

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.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new9.73 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,449 pass(es).
[ View ]

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.

Xano’s picture

Title:Remove \Drupal\Core\Plugin\Mapper\MapperInterface and merge its functionality into the few interfaces that need it» Make \Drupal\Core\Plugin\Mapper\MapperInterface optional for plugin managers
Xano’s picture

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

Xano queued 59: drupal_1894130_59.patch for re-testing.

msonnabaum’s picture

Since this was originally my issue, I just thought I'd comment to say that the latest proposal is not really a step forward. The entire idea was to remove the interface because it was unnecessary. Keeping it around in places doesn't exactly work towards that goal.

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

Xano’s picture

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?

Xano’s picture

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.

Xano queued 59: drupal_1894130_59.patch for re-testing.

dawehner’s picture

I agree, making it optional is better than nothing.

Xano’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 59: drupal_1894130_59.patch, failed testing.

Status:Needs work» Needs review

Xano queued 59: drupal_1894130_59.patch for re-testing.

Xano’s picture

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

Xano’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC per #69 and #72.

msonnabaum’s picture

Agreed with #67. Not ideal, but better than doing nothing.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs change record

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.

Xano’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs change record

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

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

Xano’s picture

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.

neclimdul’s picture

We should probably update the CR to include that.

Xano’s picture

The CR now includes this.

neclimdul’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 78: drupal_1894130_78.patch, failed testing.

Status:Needs work» Needs review

neclimdul queued 78: drupal_1894130_78.patch for re-testing.

Xano’s picture

Status:Needs review» Reviewed & tested by the community

Random test fail.

alexpott’s picture

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.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

For #85.

Xano’s picture

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

xjm’s picture

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

Xano’s picture

Issue summary:View changes

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.

Xano’s picture

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.