Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Title: Add implementing module to plugin data on display handlers » Add module owner to plugin data on display handlers
damiankloip’s picture

FileSize
913 bytes

This should be very quick to implement, we just add this to addItem.

Now we just need to add this to all the current YAML files.... *Looks at dawehner*

dawehner’s picture

Status: Active » Needs review
FileSize
55.71 KB
1.34 KB

Updated all the yml files with the script in the attachment.

Just linking an issue with does something related but at the exact space in code. #1810480: Provide the plugin_id to support views metadata integration

Status: Needs review » Needs work

The last submitted patch, drupal-1825896-3.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
53.87 KB

Let's postpone this on #1810480: Provide the plugin_id to support views metadata integration as it requires the same kind of assertions.

dawehner’s picture

Status: Needs review » Postponed

.

jibran’s picture

Status: Postponed » Active

As per #5

xjm’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
1.22 KB
95.99 KB

Here is an initial patch, which also fixes some of the yml files, so my script could work.

Status: Needs review » Needs work

The last submitted patch, drupal-1825896-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
95.97 KB

Just a rerole. (frontpage got changed)

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -VDC

The last submitted patch, drupal-1825896-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#11: drupal-1825896-11.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience), +VDC

The last submitted patch, drupal-1825896-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
95.98 KB

Yet another one.

Status: Needs review » Needs work

The last submitted patch, drupal-1825896-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
96.62 KB

It's great to see that our test coverage works properly.

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -VDC

The last submitted patch, drupal-1825896-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +VDC

#17: drupal-1825896-17.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs 100% another rerole/another run of the script.

damiankloip’s picture

Status: Needs work » Postponed

It's not that easy to get the actual module owner anymore, so I opened #2022087: Add module owner to plugin definition in AnnotatedClassDiscovery. This is pretty dependent on that I would say.

jibran’s picture

Status: Postponed » Needs work
damiankloip’s picture

Status: Needs work » Needs review
FileSize
121.24 KB

OK, here goes.....

Now that issue dep (in #21/#22) is in, we can try this again. After that got in though, I realised that views' ViewsHandlerDiscovery actually extends the Component AnnotatedClassDiscovery and not the Core implementation, so we also need to change that too.

To make things easier, these are the changes in this patch that have been made to ViewsHandlerDiscovery:

diff --git a/core/modules/views/lib/Drupal/views/Plugin/Discovery/ViewsHandlerDiscovery.php b/core/modules/views/lib/Drupal/views/Plugin/Discovery/ViewsHandlerDiscovery.php
index 51664cb..460ef5a 100644
--- a/core/modules/views/lib/Drupal/views/Plugin/Discovery/ViewsHandlerDiscovery.php
+++ b/core/modules/views/lib/Drupal/views/Plugin/Discovery/ViewsHandlerDiscovery.php
@@ -7,7 +7,7 @@
 
 namespace Drupal\views\Plugin\Discovery;
 
-use Drupal\Component\Plugin\Discovery\AnnotatedClassDiscovery;
+use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery;
 
 /**
  * Defines a discovery mechanism to find Views handlers in PSR-0 namespaces.
@@ -48,7 +48,10 @@ function __construct($type, \Traversable $root_namespaces) {
     foreach ($root_namespaces as $namespace => $dir) {
       $plugin_namespaces["$namespace\\Plugin\\views\\{$type}"] = array($dir);
     }
-    parent::__construct($plugin_namespaces, $annotation_namespaces, 'Drupal\Component\Annotation\PluginID');
+
+    $this->pluginNamespaces = $plugin_namespaces;
+    $this->annotationNamespaces = $annotation_namespaces;
+    $this->pluginDefinitionAnnotationName = 'Drupal\Component\Annotation\PluginID';
   }
 
   /**

The rest is just alot of small yaml changes.

Status: Needs review » Needs work

The last submitted patch, 1825896-23.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.36 KB
125.75 KB

And with the actual code to add plugin ID to newly added handlers.

I also had to fix the failing UserDataTest. When I changed/added the module key for handlers, this failed as the options were storing 'module' for the module that user data should be loaded for. I changed these options to 'data_module' and 'data_name' - should pass now.

damiankloip’s picture

Issue tags: -Needs reroll

.

Status: Needs review » Needs work

The last submitted patch, 1825896-24.patch, failed testing.

damiankloip’s picture

If changes in #2022087: Add module owner to plugin definition in AnnotatedClassDiscovery get in, we maybe want to change this too.

xjm’s picture

heddn’s picture

Status: Needs work » Needs review
FileSize
131.96 KB

Let's see how this goes.

Status: Needs review » Needs work

The last submitted patch, drupal-Views-module-owner-1825896-31.patch, failed testing.

damiankloip’s picture

#30, see comments in #28/#29

heddn’s picture

Status: Needs work » Needs review
FileSize
123.74 KB
133.14 KB

This seems to get us closer. But there are probably still a few errors on tests.

Status: Needs review » Needs work

The last submitted patch, drupal-Views-module-owner-1825896-33.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
134.26 KB

I fixed an overlooked omission in views.view.test_user_data.yml from #25. Then I had to make some changes in WizardPluginBase.php. The changes made make sense to me, but I'd like another set of eyes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in as fast as possible!

tim.plunkett’s picture

Priority: Normal » Major
Issue tags: +Avoid commit conflicts

Not sure if this tag does anything these days.

Because #1822048: Introduce a generic fallback plugin mechanism has left us in an unshippable state, and this blocks it, bumping.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.59 KB
133.67 KB

Sorry, just a couple of things; The assignment of the provider in WizardPluginBase was still checking for 'module', there were also some missing 'provider' keys in the wizard extending classes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

damiankloip’s picture

Title: Add module owner to plugin data on display handlers » Add module owner to plugin data on handlers
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Avoid commit conflicts

Committed a4c65be and pushed to 8.x. Thanks!

andypost’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/wizard/Comment.phpundefined
@@ -54,12 +54,14 @@ class Comment extends WizardPluginBase {
-      'field' => 'status'
+      'field' => 'status',
+      'provider' => 'user'
...
       'field' => 'status',
+      'provider' => 'user',

This change is wrong! needs follow-up

dawehner’s picture

Please post a new issue and tag it as novice.

andypost’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.