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.
Comment | File | Size | Author |
---|---|---|---|
#9 | 2096787-10.patch | 15.03 KB | damiankloip |
#9 | interdiff-2096787-10.txt | 911 bytes | damiankloip |
#6 | 2096787-6-cleanup_views_area_plugin.patch | 15.11 KB | derhasi |
#6 | interdiff.txt | 1.14 KB | derhasi |
#2 | 2096787-2.patch | 15.03 KB | damiankloip |
Comments
Comment #1
dawehnerJust some fast reviews.
Well, we could also just skip that in assume that @inheritdoc would give us something useful, which is probably not the case ;)
Lazy c&p
needs docs
Nice!
Comment #2
damiankloip CreditAttribution: damiankloip commentedNice, thanks for the review!
Comment #3
dawehnerI love that you don't typehint to config ... as this are details we should not expose if possible.
Comment #4
alexpottPatch no longer applies.
Comment #5
derhasi CreditAttribution: derhasi commentedI guess being more verbose on the params definitions, would help DX a lot.
There's a missing whitespace, too.
I care about in the reroll ;)
Comment #6
derhasi CreditAttribution: derhasi commentedI rerolled the patch and changed the param documentation of
Drupal\views\Plugin\views\area\View::__construct()
to the one used in the general pluginBase. This way (at least me) could understand it better.Comment #7
damiankloip CreditAttribution: damiankloip commentedNot sure why we are changing these to be more verbose? These were just pasted from another place, so I think it's more consistent to keep that.
Comment #8
derhasi CreditAttribution: derhasi commented@damiankloip, I added it, because ...
Comment #9
damiankloip CreditAttribution: damiankloip commentedSorry, but I really think we should keep these docs the same as other places. Plus I don't think the changes in #6 really add much, we know this object is a plugin anyway..
Comment #10
dawehnerI agree with the point of damian. If we want to fix the docs we should fix it on the upper level and everywhere else.
Comment #11
derhasi CreditAttribution: derhasi commentedIf we add an additional parameter to a function, we should not make its documentation worse. Drupal\Component\Plugin\PluginBase already holds that param documentation. And that's a parent of the views area plugins.
Sure there are some developers that already are into Drupal 8 and know the basics of it and can derive information from that. But as someone digging into it, it's crucial to have good information provided at any place. And as long as there is no
@inheritdoc@
that function's should be fully documented there.Changing that lines provides some tiny information, that may help a lot of devs out, if they want to deal with the plugin in that place. It holds only benefits and no downsides.
Well, why not start here, in the scope of this issue?
Comment #12
damiankloip CreditAttribution: damiankloip commentedThe docs were totally fine as they were. You just added plugin context to the docs, which isn't needed at all. As its the constructor for a plugin anyway. This issue is about area plugins. If you want to fix that somewhere else, open a new issue.
Comment #13
derhasi CreditAttribution: derhasi commentedNo they were not. I first had to dig into the code and look around in other plugins and talk to other people, till I got the real meaning of the params. That is the reason why I changed it.
Where did you copy it from?
DX is important and it starts in the small parts. So as that change does not hurt anybody, but provides more information on the developer, I do not understand, why that is a problem? I do not want to tread on your toes, I just want to improve things.
Comment #14
damiankloip CreditAttribution: damiankloip commentedSorry, I just don't see how 'The plugin_id for the plugin instance.' is more informative than 'The plugin ID' for the constructor of a plugin class. If anything that is less correct because plugin_id is just naming the variable, not the pluginId property or 'id' from the definition.
Comment #15
dawehner@damian
There is no reason to fight about this. The parent class Component\PluginBase exactly has this piece of documentation so this is kind of a cleanup to keep the documentation in sync.
I think this is a cleanup which is the main reason of the issue. I hope you are fine with that.
Comment #16
damiankloip CreditAttribution: damiankloip commentedOK, I don't mind. Let's just go with the patch in #6. I'm not really a fan of wasting this much time arguing about docs :-)
Sorry derhasi, I'll let you take this one home!
Comment #17
damiankloip CreditAttribution: damiankloip commentedLet's RTBC patch in #6.
Comment #18
damiankloip CreditAttribution: damiankloip commentedComment #19
dawehnerThank you!
Comment #20
catchCommitted/pushed #6 to 8.x, thanks!