Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

With #1969388: Change notice: Add dedicated annotations for Views plugins we'll only have to remove it in one place.

That doesn't cover the actual usages of 'module', though.

damiankloip’s picture

Title: Remove obsolete module keys from Views plugin annotations » Remove obsolete "module" keys from Views plugin annotations

Huh? This issue is specifically for that. I guess module key and "module" key are different ;)

What did you think this meant? I'm confused. The issue you mentioned will not keep a default module key in the annotation, just provider.

tstoeckler’s picture

Yeah, I just realized #1 was pointless. Nevermind me :-)

That issue will still keep 'module', though.

damiankloip’s picture

Only in the actual annotations though, right? As @Plugin is generic.

The new annotations will not include this default. So this is kind of a housekeeping task I guess.

tstoeckler’s picture

Sorry, I think I'm not getting your point.

From the patch over there:

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPluginAnnotationBase.php
@@ -0,0 +1,41 @@
+abstract class ViewsPluginAnnotationBase extends Plugin implements AnnotationInterface {
...
+  /**
+   * The module the plugin is defined by.
+   *
+   * @var string
+   */
+  public $module = 'views';
...
+}

So the $module / 'module' key is still in the annotations. The actual plugins still contain it as well. Those need to be removed. And everywhere we check for $plugin_definition['module'] needs to be changed to $plugin_definition['provider'], accordingly.

damiankloip’s picture

I'm not sure we need that in the new annotation classes, but we might as well remove it when we tidy module stuff up I guess.

The trouble is that we currently still use a module key on the actual View entity object to store the module that provided the view, separate from plugins.

Also, not sure we can actually make the switch as there are things like blocks, that views will extends (like BlockBase) that still assume a module key.

tstoeckler’s picture

Status: Active » Needs review
FileSize
670 bytes

Let's see what breaks with this.

mondrake’s picture

Status: Needs review » Needs work

The 'Views plugins' report under admin/reports/views-plugins calls views_plugin_list() which uses the 'module' key to report the provider, instead of the 'provider' one. Shall that be aligned?

tstoeckler’s picture

Yes, certainly.

The fact that the patch is green, however, means that anything that needs to be adjusted is completely untested, so that we will need to add tests here.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
582 bytes
670 bytes

Not necessarily, as most plugins currently declare the 'module' key themselves; as well as in the plugin manager - There is still a module key in the defaults array there.

See how this goes, Possibly some failures, not sure.

damiankloip’s picture

FileSize
1.22 KB

Oops, sorry. Uploaded the wrong patch in #10. interdiff is good.

tstoeckler’s picture

Ahh, I didn't realize that, thanks.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That is totally fine as it is!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

Nope, #8 was correct. You get a

Notice: Undefined index: module in views_plugin_list() (line 237 of core/modules/views/views.module).

on /admin/reports/views-plugins with the patch applied (after clearing caches!).

damiankloip’s picture

Status: Needs work » Needs review
FileSize
17.47 KB
18.69 KB

I want this issue to also remove them from the annotations too, then we have excised the use of 'module' keys in views plugins.

tstoeckler’s picture

+++ b/core/modules/views/views.module
+++ b/core/modules/views/views.module
@@ -234,7 +234,7 @@ function views_plugin_list() {
-            'module' => check_plain($info[$name]['module']),
+            'provider' => check_plain($info[$name]['provider']),

I'm not really sure if "Provider" would confuse people, but in theory we should also rename the column heading to "Provider". Or we could decide to leave it that way for UX, although I'm not really sure on that.

damiankloip’s picture

Issue tags: +Needs usability review

Good question, and I'm not sure on that. We only really use provider on the backend now, but I think that's more of a plugin thing - to keep it as generic as possible.

Tagging with need usability review, someone there can tell us whether to change Module in the table header or not.

mondrake’s picture

FileSize
33.87 KB

Not sure I understand... in the 'Views plugins' report right now in HEAD we already have the column header = 'Provided by'.

So what would be the change? Or are we talking about sth different?

Untitled.png

tstoeckler’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review

Oops, thanks @mondrake for clearing that up. That means there's nothing to change.

Still needs tests for that page, however.

damiankloip’s picture

Seems like it's a bit out of scope of this issue to add tests for that page though?

tstoeckler’s picture

Well, I guess in the end that's for the committers to decide, but in my opinion if we break things with a passing patch we could just as well have broken it in HEAD, so IMO test coverage is obligatory.

I'll see if I can coop up something later today.

damiankloip’s picture

OK, great! We should only need a DUTB (I think) test for views_plugin_list(). You could actually just add a new method in Drupal\views\Tests\ModuleTest.

tstoeckler’s picture

Sadly, I didn't get to it, and I won't for the next 2 weeks as I'm completely AFK. Sorry! :-/

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
20.74 KB

No worries :)

Here is a test for the views_plugin_list() function.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Removes obsoleted element, has tests, all feedback addressed. RTBC for me.

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/ModuleTest.php
@@ -255,6 +256,26 @@ public function testViewsFetchPluginNames() {
+    // Only data from 'test_view' should be contained in the plugin list.
+    foreach (array('display:default', 'pager:none') as $key) {

Wait, aren't there also plugins from at least the views module? This comment sounds confusing.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Novice
FileSize
820 bytes
20.74 KB

Fair enough, how about this?

mondrake’s picture

#27: 2070609-27.patch queued for re-testing.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

#24 was RTBC, #27 just changed a comment. Seems ready IMHO.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 11eeb98 and pushed to 8.x. Thanks!

alexpott’s picture

Committed 11eeb98 and pushed to 8.x. Thanks!

tstoeckler’s picture

Yay, thanks @damiankloip for finishing this up! Awesome!!!

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