Currently there's no way to alter plugin definitions because MigratePluginManager does not use DefaultPluginManager::getDefinition()

CommentFileSizeAuthor
migrate_alter.patch1.01 KBandypost

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I wonder whether we need a test for that?

chx’s picture

chx commented

I somewhat doubt. Very nice catch, thanks andypost.

chx’s picture

chx commented

To elaborate: theoretically a test might be useful but I am afraid it'd take a lot of work to test that the plugin alteration works and it's not worth it. Usually we want a test to make sure we do not regress but regressing on this? That looks extremely unlikely.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Quick fix

I think we have enough test coverage for DefaultPluginManager
PS: to test this properly migrate needs test module with alter hook, not sure it makes sense to implement one to test default functionality

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Hm. I really don't like committing bug fixes without tests, but basically chx/andypost have pointed out that the base plugin classes to contain test coverage for this, and if we added an alter test here it'd be a one-off for this single plugin type, which doesn't really make much sense. The bug was caused by a typo that's unlikely to be re-introduced.

So, with that, committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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