Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Needs review » Needs work

Just some fast reviews.

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Broken.php
    @@ -26,11 +26,22 @@ public function adminLabel($short = FALSE) {
    +  /**
    +   * @return string|void
    +   */
    

    Well, we could also just skip that in assume that @inheritdoc would give us something useful, which is probably not the case ;)

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/View.php
    @@ -18,6 +19,30 @@
    +  /**
    +   * Constructs a Plugin object.
    +   */
    

    Lazy c&p

  3. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/View.php
    @@ -18,6 +19,30 @@
    +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityStorageControllerInterface $view_storage) {
    ...
    +    $this->viewStorage = $view_storage;
    

    needs docs

  4. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/View.php
    @@ -54,13 +78,14 @@ public function buildOptionsForm(&$form, &$form_state) {
    -      $view = views_get_view($view_name);
    +      $view = $this->viewStorage->load($view_name);
    

    Nice!

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
15.03 KB

Nice, thanks for the review!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/View.php
@@ -20,7 +20,23 @@
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
...
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $view_storage

I love that you don't typehint to config ... as this are details we should not expose if possible.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

derhasi’s picture

Assigned: Unassigned » derhasi
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/View.php
@@ -18,6 +19,46 @@
+   * @param array $configuration
+   *   An array of configuration data.
+   * @param string $plugin_id
+   *   The plugin ID.
+   * @param array $plugin_definition
+   *   The plugin definition.
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $view_storage
+   *  The view storage controller.

I 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 ;)

derhasi’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.14 KB
15.11 KB

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

damiankloip’s picture

Assigned: derhasi » Unassigned
Status: Needs review » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/View.php
@@ -37,13 +37,13 @@ class View extends AreaPluginBase {
+    *   A configuration array containing information about the plugin instance.
...
+    *   The plugin_id for the plugin instance.
...
+    *   The plugin implementation definition.
...
+    *   The view storage controller.

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

derhasi’s picture

@damiankloip, I added it, because ...

  1. ... the previous one would not be specific enough: plugin ID of what? configuration of what?. Adding these small additions, would have made it easer for me to dive in the code and understand it in the first place.
  2. ... Drupal\Component\Plugin\PluginBase is the first parent to document the existing parameters, which documentation seem sufficient to me. As we do not have a "@partiallyinheritdoc@ I guess copying it from a parent is more consistent.
damiankloip’s picture

Status: Needs work » Needs review
FileSize
911 bytes
15.03 KB

Sorry, 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..

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I agree with the point of damian. If we want to fix the docs we should fix it on the upper level and everywhere else.

derhasi’s picture

Status: Reviewed & tested by the community » Needs work

.. Drupal\Component\Plugin\PluginBase is the first parent to document the existing parameters, which documentation seem sufficient to me. As we do not have a "@partiallyinheritdoc@ I guess copying it from a parent is more consistent.

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

If we want to fix the docs we should fix it on the upper level and everywhere else.

Well, why not start here, in the scope of this issue?

damiankloip’s picture

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

derhasi’s picture

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

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

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.

damiankloip’s picture

Sorry, 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.

dawehner’s picture

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

damiankloip’s picture

OK, 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!

damiankloip’s picture

Let's RTBC patch in #6.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community
dawehner’s picture

Thank you!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed #6 to 8.x, thanks!

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