Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

Status: Active » Needs review
dawehner’s picture

Status: Needs review » Needs work

Thanks for working on that!

+++ b/lib/Drupal/views/Plugin/views/access/AccessPluginBase.phpundefined
@@ -11,28 +11,24 @@ use Drupal\views\Plugin\views\PluginBase;
- * @defgroup views_access_plugins Views access plugins

What is the reason to remove the defgroup?

+++ b/lib/Drupal/views/Plugin/views/access/AccessPluginBase.phpundefined
@@ -11,28 +11,24 @@ use Drupal\views\Plugin\views\PluginBase;
+ * to implement the access and the getAccessCallback() method.

We probably want to have () on access as well.

+++ b/lib/Drupal/views/Plugin/views/access/None.phpundefined
@@ -23,6 +23,9 @@ use Drupal\Core\Annotation\Plugin;
+  /**
+   * Implements Drupal\views\Plugin\views\access\AccessPluginBase::summaryTitle().
+   */

You know what i don't get: when to use Implements and when to use Overriddes.

aspilicious’s picture

1) This is my understanding of @defgroup/@ingroup, I can be wrong as the docs aren't 100% clear

Defgroup is used when you want to define an entire group of similar functions at once, when all the access plugins are in one file we can wrap them in a defgroup

@defgroup {
....
}

In our case the plugins are in seperate files so we just need to mark each of them being of the same group. Thats why we use @ingroup.

-------------------------------------------------------

2) When you're using @overrides, you override existing functinality, @implements is for implementing code for the first time. For example when you implement methods that are defined abstract in the parent. I see I made a mistake with "public function summaryTitle() {" as it isn't abstract method so we should use @overrides on those.

xjm’s picture

Thanks @aspilicious. Few thoughts though:

+++ b/lib/Drupal/views/Plugin/views/access/AccessPluginBase.phpundefined
@@ -11,28 +11,24 @@ use Drupal\views\Plugin\views\PluginBase;
 /**
- * @defgroup views_access_plugins Views access plugins
...
  * The base plugin to handle access to a view.
...
+ * @ingroup views_access_plugins

The patch is deleting the group definition, but then adding an @ingroup to that group. Doesn't make any sense. :)

I have a vague plan on how to clean up the doc groups later, but it's a post-feature-freeze task. So is renaming methods to camelCase(); we absolutely should not do that yet.

However, the method documentation cleanups in this patch are useful before then. Adding missing docblocks or parameter or return documentation are definitely worthwhile, but I'd like to keep anything else out of scope for now.

xjm’s picture

So from a draft post somewhere else, this is my version of the documentation gate:

One-line summary for all classes, constants, functions, methods, and files; @param, @return, @var, and @throws, all with datatypes; inline comments; and explanations for anything complicated.

I'd prefer not to have any docs or code style polish outside those things before feature freeze.

aspilicious’s picture

Can you explain to me the difference between @defgroup and @ingroup than?

In the past we had one file:

@defgroup something {

class1

class2

class3

...

}

Now we have seperate files for each class so I thought this should be:

@ingroup something
class 1

// new file

@ingroup something
class 2

// new file

.....

xjm’s picture

@aspilicious, sure, but an @ingroup that has no @defgroup is broken. There needs to be a @defgroup somewhere, and then the classes that belong in that group either can be inside it, or elsewhere and then added to it with @ingroup. So actually the current format (with a defgroup wrapping a single class in a PSR-0 file) is probably fine for the main plugin class, and then classes that extend it can get @ingroup.

Alternately, we could move the @defgroup to views.api.php, and put the @ingroup on the main plugin type class as well. There are advantages and disadvantages to that. The main disadvantage would be that we might end up duplicating documentation that should go in the class docblock for the mainplugin type class.

xjm’s picture

But, to clarify, I'd rather not bother with implementing a change in that regard until at least feature freeze. :)

xjm’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Status: Needs work » Postponed
Issue tags: +VDC
mgifford’s picture

Issue summary: View changes
Status: Postponed » Active

Post feature freeze.

jhodgdon’s picture

Um... What are we doing here? This issue is old. The Views plugin topics have been written, and all plugin type docs were cleaned up considerably on #2269389: [meta] Make sure plugin developer info is discoverable. Do we still need this issue?

mgifford’s picture

Status: Active » Closed (fixed)

I'm just cleaning up old issues in the queue that had been left open and marked Postponed. We can mark this closed for now.