We now use the 'provider' key, and don't explicitly ned this declared in each annotation.

Remove them all please.

Files: 
CommentFileSizeAuthor
#27 2070609-27.patch20.74 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,004 pass(es).
[ View ]
#27 interdiff-2070609-27.txt820 bytesdamiankloip
#24 2070609-24.patch20.74 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,317 pass(es).
[ View ]
#24 interdiff-2070609-24.txt2.05 KBdamiankloip
#18 Untitled.png33.87 KBmondrake
#15 2070609-15.patch18.69 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,268 pass(es).
[ View ]
#15 interdiff-2070609-15.txt17.47 KBdamiankloip
#11 2070609-11.patch1.22 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,752 pass(es).
[ View ]
#10 2070609-10.patch670 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,613 pass(es).
[ View ]
#10 interdiff-2070609-10.txt582 bytesdamiankloip
#7 2070609-7-views-plugins-no-module.patch670 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,510 pass(es).
[ View ]

Comments

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.

Title:Remove obsolete module keys from Views plugin annotationsRemove 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.

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

That issue will still keep 'module', though.

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.

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.

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.

Status:Active» Needs review
StatusFileSize
new670 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,510 pass(es).
[ View ]

Let's see what breaks with this.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new582 bytes
new670 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,613 pass(es).
[ View ]

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.

StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,752 pass(es).
[ View ]

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

Ahh, I didn't realize that, thanks.

Status:Needs review» Reviewed & tested by the community

That is totally fine as it is!

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!).

Status:Needs work» Needs review
StatusFileSize
new17.47 KB
new18.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,268 pass(es).
[ View ]

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

+++ 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.

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.

StatusFileSize
new33.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

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.

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

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new2.05 KB
new20.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,317 pass(es).
[ View ]

No worries :)

Here is a test for the views_plugin_list() function.

Status:Needs review» Reviewed & tested by the community

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

+++ 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.

Status:Reviewed & tested by the community» Needs review
Issue tags:-Novice
StatusFileSize
new820 bytes
new20.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,004 pass(es).
[ View ]

Fair enough, how about this?

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

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

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

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

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