Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We now use the 'provider' key, and don't explicitly ned this declared in each annotation.
Remove them all please.
Comment | File | Size | Author |
---|---|---|---|
#27 | 2070609-27.patch | 20.74 KB | damiankloip |
#27 | interdiff-2070609-27.txt | 820 bytes | damiankloip |
#24 | 2070609-24.patch | 20.74 KB | damiankloip |
#24 | interdiff-2070609-24.txt | 2.05 KB | damiankloip |
#18 | Untitled.png | 33.87 KB | mondrake |
Comments
Comment #1
tstoecklerWith #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.
Comment #2
damiankloip CreditAttribution: damiankloip commentedHuh? 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.
Comment #3
tstoecklerYeah, I just realized #1 was pointless. Nevermind me :-)
That issue will still keep 'module', though.
Comment #4
damiankloip CreditAttribution: damiankloip commentedOnly 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.
Comment #5
tstoecklerSorry, I think I'm not getting your point.
From the patch over there:
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.
Comment #6
damiankloip CreditAttribution: damiankloip commentedI'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.
Comment #7
tstoecklerLet's see what breaks with this.
Comment #8
mondrakeThe '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?
Comment #9
tstoecklerYes, 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.
Comment #10
damiankloip CreditAttribution: damiankloip commentedNot 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.
Comment #11
damiankloip CreditAttribution: damiankloip commentedOops, sorry. Uploaded the wrong patch in #10. interdiff is good.
Comment #12
tstoecklerAhh, I didn't realize that, thanks.
Comment #13
dawehnerThat is totally fine as it is!
Comment #14
tstoecklerNope, #8 was correct. You get a
on /admin/reports/views-plugins with the patch applied (after clearing caches!).
Comment #15
damiankloip CreditAttribution: damiankloip commentedI want this issue to also remove them from the annotations too, then we have excised the use of 'module' keys in views plugins.
Comment #16
tstoecklerI'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.
Comment #17
damiankloip CreditAttribution: damiankloip commentedGood 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.
Comment #18
mondrakeNot 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?
Comment #19
tstoecklerOops, thanks @mondrake for clearing that up. That means there's nothing to change.
Still needs tests for that page, however.
Comment #20
damiankloip CreditAttribution: damiankloip commentedSeems like it's a bit out of scope of this issue to add tests for that page though?
Comment #21
tstoecklerWell, 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.
Comment #22
damiankloip CreditAttribution: damiankloip commentedOK, 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.
Comment #23
tstoecklerSadly, I didn't get to it, and I won't for the next 2 weeks as I'm completely AFK. Sorry! :-/
Comment #24
damiankloip CreditAttribution: damiankloip commentedNo worries :)
Here is a test for the views_plugin_list() function.
Comment #25
mondrakeRemoves obsoleted element, has tests, all feedback addressed. RTBC for me.
Comment #26
dawehnerWait, aren't there also plugins from at least the views module? This comment sounds confusing.
Comment #27
damiankloip CreditAttribution: damiankloip commentedFair enough, how about this?
Comment #28
mondrake#27: 2070609-27.patch queued for re-testing.
Comment #29
mondrake#24 was RTBC, #27 just changed a comment. Seems ready IMHO.
Comment #30
alexpottCommitted 11eeb98 and pushed to 8.x. Thanks!
Comment #31
alexpottCommitted 11eeb98 and pushed to 8.x. Thanks!
Comment #32
tstoecklerYay, thanks @damiankloip for finishing this up! Awesome!!!