Part of #2010412: [Meta] Change plugin managers to use DefaultPluginManager

CommentFileSizeAuthor
#11 inplaceeditor-default-plugin-manager-2204621-11-interdiff.txt957 bytesberdir
#11 inplaceeditor-default-plugin-manager-2204621-11.patch5.83 KBberdir
#9 inplaceeditor-default-plugin-manager-2204621-9.patch4.89 KBberdir
#6 inplaceeditor-default-plugin-manager-2204621-6-interdiff.txt2 KBberdir
#6 inplaceeditor-default-plugin-manager-2204621-6.patch4.89 KBberdir
#4 inplaceeditor-default-plugin-manager-2204621-4-interdiff.txt2.72 KBberdir
#4 inplaceeditor-default-plugin-manager-2204621-4.patch4.92 KBberdir
#1 inplaceeditor-default-plugin-manager-2204621-1.patch3.09 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new inplaceeditor-default-plugin-manager-2204621-1.patch3.09 KB

Status: Needs review » Needs work

The last submitted patch, 1: inplaceeditor-default-plugin-manager-2204621-1.patch, failed testing.

The last submitted patch, 1: inplaceeditor-default-plugin-manager-2204621-1.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new inplaceeditor-default-plugin-manager-2204621-4.patch4.92 KB
new inplaceeditor-default-plugin-manager-2204621-4-interdiff.txt2.72 KB

Fixing tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/editor/lib/Drupal/editor/Tests/EditIntegrationTest.php
@@ -124,7 +124,7 @@ protected function getSelectedEditor($entity_id, $field_name, $view_mode = 'defa
-    $this->editorManager = new InPlaceEditorManager($this->container->get('container.namespaces'));
+    $this->editorManager = new InPlaceEditorManager($this->container->get('container.namespaces'), $this->container->get('cache.cache'), $this->container->get('language_manager'), $this->container->get('module_handler'));

@@ -151,7 +151,7 @@ public function testEditorSelection() {
-    $this->editorManager = new InPlaceEditorManager($this->container->get('container.namespaces'));
+    $this->editorManager = new InPlaceEditorManager($this->container->get('container.namespaces'), $this->container->get('cache.cache'), $this->container->get('language_manager'), $this->container->get('module_handler'));

Why not just use the service here?

Otherwise, RTBC.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new inplaceeditor-default-plugin-manager-2204621-6.patch4.89 KB
new inplaceeditor-default-plugin-manager-2204621-6-interdiff.txt2 KB

Yeah, this might have made sense when there were no dependencies, but it's pointless now. Kept it for the MetadataGenerator, as that one needs a mocked access checker.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Plugin system

Great, thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

RTBC +1, thanks!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new inplaceeditor-default-plugin-manager-2204621-9.patch4.89 KB

Re-roll after #2053153: Allow contrib modules to provide plugins on behalf of optional modules

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: inplaceeditor-default-plugin-manager-2204621-9.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new inplaceeditor-default-plugin-manager-2204621-11.patch5.83 KB
new inplaceeditor-default-plugin-manager-2204621-11-interdiff.txt957 bytes

Ah, the new feature now gets in the way and removes that definition because it comes from a module that the injected instance of the module handler doesn't know about. Either way, updating the objects helps. This works right now in HEAD because only the default plugin manager has that logic built in.

The whole class is kind of a strange mix of a DUBT test and a unit test by mixing the use of manually created objects and objects from the container and custom mocks, and assigning them to properties, so that they are not updated when module list changes.

I think it should either always call $this->container->get('plugin.manager.edit.editor') etc. directly, probably use a way to grant actual permissions to the current user, so that the normal access checker works, or be converted to a real unit test where everything except the class being tested is mocked. That definitely wasn't possible when it was originally written, but I think we're much closer to that being possible now :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Agreed :) But for now, this is RTBC, converting to a PHPUnit test can be a follow-up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 8e47a4e and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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