Problem/Motivation

Now that #2010412: [Meta] Change plugin managers to use DefaultPluginManager is done, nothing is really using those two anymore, so we should remove them.

Proposed resolution

Remove the two classes, convert the remaining usage in the test plugin manager.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
30.22 KB

First patch, removing a bunch of tests and test code that I think we no longer need because we have enough test coverage through unit tests and integration tests with the actual plugin manager.

Also removed CachedDiscoveryInterface from PluginManagerBase because there's no discovery that implements that anymore.

dawehner’s picture

I don't get why we always have to follow the rule: if core does not use it, there is no use for it. Using decorators is a great idea in the first place, because it shows the flexiblity of the system.

Berdir’s picture

Because we agreed that those three are a bad pattern, CacheDecorator is the reason why we had to add ProcessDecorator/AlterDecorator so that we can move that inside the cache. And we did that even though EclipseGc and neclimdul were against it.

Which is why we created DefaultPluginManager.

So my reason for removing this is that we should not promote a bad pattern, and it's 700 lines of code less to maintain.

But mostly, I created this issue to start the discussion if we want to do this and/or only parts of this.

EclipseGc’s picture

Yeah, consider me ++ to this in general. This is the reason the DefaultPluginManager was created for Drupal's use, and that has fixed a TON of problems. Removing the temptation to perpetuate a pattern we worked hard to exterminate seems ++ to me. Understand I completely agree with dawehner in principle, but in this particular case, I am completely on board with removing this code since it's not a utility whose use we wish to perpetuate.

Eclipse

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Okay, I am convinced as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: remove-decorators-2271363-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.71 KB

Looks like you took too long to get convinced ;)

Here's a re-roll.

Status: Needs review » Needs work

The last submitted patch, 7: remove-decorators-2271363-7.patch, failed testing.

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

see above

alexpott’s picture

We need to remove https://www.drupal.org/node/1887798 and check any docs that exist on d.o. :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0fcb80b and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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