Similar to #1965164: Add a @PluginID annotation for plugins that only need an ID, like Views handlers let's also add additional annotations for all views plugins.

This belongs to the following meta issue: #1966246: [meta] Introduce specific annotations for each plugin type

The mapping for the different kind of plugin types is the following:

Plugin type Annotation class
Display plugin \Drupal\views\Plugin\Annotation\ViewsDisplay
Cache plugin \Drupal\views\Plugin\Annotation\ViewsCache
Display Extender plugin \Drupal\views\Plugin\Annotation\ViewsDisplayExtender
Exposed Form plugin \Drupal\views\Plugin\Annotation\ViewsExposedForm
Query plugin \Drupal\views\Plugin\Annotation\ViewsQuery
Argument default plugin \Drupal\views\Plugin\Annotation\ViewsArgumentDefault
Argument validator plugin \Drupal\views\Plugin\Annotation\ViewsArgumentValidator
Access \Drupal\views\Plugin\Annotation\ViewsAccess
Style \Drupal\views\Plugin\Annotation\ViewsStyle
Row \Drupal\views\Plugin\Annotation\ViewsRow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Assigned: Unassigned » dawehner

Assign

dawehner’s picture

Project: Drupal core » Views (for Drupal 7)
Issue summary: View changes

blub

dawehner’s picture

Issue summary: View changes

added some annotations

dawehner’s picture

Issue summary: View changes

removed usage

dawehner’s picture

Project: Views (for Drupal 7) » Drupal core
FileSize
83.22 KB

That's currently blocked on #1881606: Use a derivative to integrate all entities as row plugins as it puts in annotations, which we should not support for all row plugins.

dawehner’s picture

Project: Drupal core » Views (for Drupal 7)
Issue summary: View changes

a

dawehner’s picture

Project: Views (for Drupal 7) » Drupal core
FileSize
83.14 KB

Just a rerole, though this is still blocked on the other issue.

This moves the annotation classes into Drupal\views\Annotation
It would be cool if someone could have a look at the dependencies of this patch.

dawehner’s picture

Status: Active » Needs review
FileSize
80.12 KB

Here is a rerole now that the other patch landed.

Status: Needs review » Needs work

The last submitted patch, drupal-1969388-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
822 bytes
80.2 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
80.77 KB

Just a rerole.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-8.patch, failed testing.

olli’s picture

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -17,13 +17,12 @@
- *   uses_hook_block = TRUE,

Isn't this still needed?

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsDisplay.php
@@ -0,0 +1,116 @@
+    return array(
+      'id' => $this->id,
+      'title' => $this->title->get(),
+      'admin' => isset($this->admin) ? $this->admin->get() : '',
+      'help' => $this->help->get(),
+      'uses_hook_menu' => $this->uses_hook_menu,
+      'contextual_links_locations' => $this->contextual_links_locations,
+      'base' => $this->base,
+      'theme' => $this->theme,
+      'no_ui' => $this->no_ui,
+    );

missing 'uses_route'?

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPager.php
@@ -0,0 +1,92 @@
+    return array(
+      'id' => $this->id,
+      'title' => $this->title->get(),
+      'help' => $this->help->get(),
+      'theme' => $this->theme,
+      'display_types' => $this->display_types,
+      'base' => $this->base,
+      'no_ui' => $this->no_ui,
+    );

missing 'short_title'?

+++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsPluginManager.php
@@ -31,7 +33,13 @@ class ViewsPluginManager extends PluginManagerBase {
+      'Drupal\views\Plugin\Annotation' => $namespaces['views'],

Extra '\Plugin' and views dir is in $namespaces['Drupal\views'] i think.

+++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsPluginManager.php
@@ -31,7 +33,13 @@ class ViewsPluginManager extends PluginManagerBase {
+    $plugin_definition_annotation_name = 'Drupal\views\Plugin\Annotation\\Views' . Container::camelize($type);

Extra '\Plugin' and '\\Views' -> '\Views'

+++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsPluginManager.php
@@ -31,7 +33,13 @@ class ViewsPluginManager extends PluginManagerBase {
+    $this->discovery = new AnnotatedClassDiscovery("views\$type", $namespaces, $annotation_namespaces, $plugin_definition_annotation_name);

"views\$type" -> "views/$type"

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
81.45 KB

This was a really really good review, thank you very much.

The key "no_uid" is not used at all anywhere, so let's skip it.

This now at least installs drupal, let's see how much else is left.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
82.03 KB

Fixed a couple of missing annotations.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-13.patch, failed testing.

olli’s picture

Right, that installs! One problem.., we need to pass derivative and module in get().

Also, looks like that short_title didn't get into viewspager::get() and the generic entity row plugin is missing too?

dawehner’s picture

Status: Needs work » Needs review
FileSize
84 KB

Good catches.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-16.patch, failed testing.

olli’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/EntityRow.php
@@ -10,13 +10,13 @@
- * @Plugin(
+ * @ViewsRow(
  *   id = "entity",
  *   derivative = "Drupal\views\Plugin\Derivative\ViewsEntityRow"
  * )

This is now missing title and help. In general, do you want to make these translatables admin, title, short_title, help, ... required?

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsDisplayExtender.php
@@ -0,0 +1,68 @@
+  public function get() {
+    return array(

Why not parent::get() here?

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -VDC
FileSize
84.12 KB

Let's allow all plugins to be extended with other annotations.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.89 KB
84.67 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-21.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
619 bytes
84.7 KB

Missed one.

Status: Needs review » Needs work
Issue tags: -Plugin system, -Annotation

The last submitted patch, vdc-1969388-23.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: +Plugin system, +Annotation

#23: vdc-1969388-23.patch queued for re-testing.

dawehner’s picture

Nice!

tim.plunkett’s picture

FileSize
27.22 KB
87.04 KB

We shouldn't override get(), we can just provide the defaults in line.

Also switching to DefaultPluginManager as it lets us kill ProcessDecorator.

Status: Needs review » Needs work
Issue tags: -Plugin system, -Annotation

The last submitted patch, vdc-1969388-27.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#27: vdc-1969388-27.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Plugin system, +Annotation

The last submitted patch, vdc-1969388-27.patch, failed testing.

tim.plunkett’s picture

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -28,26 +28,28 @@ class ViewsAccess extends ViewsPluginAnnotationBase {
-   * @var \Drupal\Core\Annotation\Translation
+   * @var \Drupal\Core\Annotation\Translation (optional)
    *
    * @ingroup plugin_translatable
    */
-   public $title;
...
   /**
    * A short help string; this is displayed in the views UI.
    *
-   * @var \Drupal\Core\Annotation\Translation
+   * @var \Drupal\Core\Annotation\Translation (optional)
    *
    * @ingroup plugin_translatable
    */
-  public $help;

I know that this was done due to derivatives, but can't we skip the "(optional)" in the documentation so people don't get confused?

dawehner’s picture

FileSize
87.59 KB

Just rerolled the patch and removed optional where I think it confuses people. We are not doing that anywhere else in core as far as I know.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
720 bytes
87.34 KB

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-35.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
87.39 KB

doh!.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in! Awesome work @dawehner.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -0,0 +1,74 @@
+   * The "full" title for your access type; used in the views UI.

I don't what "full" means in this comment. In discussion with @dawehner on IRC we realised that all views plugin annotations should have short_title key since views/PluginBase::pluginTitle() supports this. I would go with The plugin title used in the views UI

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -0,0 +1,74 @@
+   * Defines the type of the display, this plugin can be used with.

Unnecessary comma and use of the word "Defines".

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -0,0 +1,74 @@
+   * Should the plugin be not selectable in the ui.

The standard here seems to be Whether or not the plugin is selectable in the ui.. I can not find any instances of Should

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -0,0 +1,74 @@
+   * If it's set to TRUE, you can still use it via the API in config files.

unnecessary "it's"

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsDisplay.phpundefined
@@ -0,0 +1,115 @@
+   * Does the display plugin uses hook_menu() to register a route to the router.

Whether or not to use hook_menu() to register a route.

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsDisplay.phpundefined
@@ -0,0 +1,115 @@
+   * An array with places where contextual links should be added. Can for
+   * example be 'page' or 'block'. If you don't specify it there will be
+   * contextual links around the rendered view. If this is not set or regions
+   * have been specified, views will display an option to 'hide contextual
+   * links'. Use an empty array if you do not want this.
A list of places where contextual links should be added.

For example:
@code
array(
  'page',
  'block',
)
@endcode

If you don't specify it there will be contextual links rendered for all displays of a view. If this is not set or regions have been specified, views will display an option to 'hide contextual links'. Use an empty array to disable.
+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPager.phpundefined
@@ -0,0 +1,90 @@
+   * The "short" title for the pager type, used in the views UI.

Comma here should be a semi-colon to be consistent.

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPluginAnnotationBase.phpundefined
@@ -0,0 +1,41 @@
+   * Defines a class which allows to provide derivatived plugins.

A class to make the plugin derivative aware.

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPluginAnnotationBase.phpundefined
@@ -0,0 +1,41 @@
+   * Determines if a theme function should be registered automatically.

Whether or not to register a theme function automatically.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Plugin system, -Annotation

#37: vdc-1969388-37.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Plugin system, +Annotation

The last submitted patch, vdc-1969388-37.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.69 KB
89.06 KB

Thanks for the good review. Here is a fix for it.

Status: Needs review » Needs work

The last submitted patch, drupal-1969388-42.patch, failed testing.

damiankloip’s picture

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -0,0 +1,83 @@
+   * The type of the display this plugin can be used with.
...
+   * For example the Feed display defines the type 'feed', so only rss style
+   * and row plugins can be used in the views UI.

We should update this docblock to reflect multiple types.

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsAccess.phpundefined
@@ -0,0 +1,83 @@
+   * Should the plugin be not selectable in the ui.

Maybe 'whether the plugin should not be selectable in the UI' or something?

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsArgumentDefault.phpundefined
@@ -0,0 +1,55 @@
+   * The plugin title used in the views UI.

Do you think if these are not optional, we should not set an empty string here?

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPluginAnnotationBase.phpundefined
@@ -0,0 +1,41 @@
+abstract class ViewsPluginAnnotationBase extends Plugin implements AnnotationInterface {

Seems like we could move more properties into here, like id, title, short_title (, help)?

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsStyle.phpundefined
@@ -0,0 +1,90 @@
+   * Should the plugin be not selectable in the ui.

Whether ...

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.17 KB
89.15 KB

Do you think if these are not optional, we should not set an empty string here?

Well, it fails otherwise with derivatives, which don't really define a title in the base implementation.

Seems like we could move more properties into here, like id, title, short_title (, help)?

We could be the full point of the special annotations is to have proper documentation per class. Otherwise we could really just use @Plugin and be happy with it.

Status: Needs review » Needs work

The last submitted patch, vdc-1969388-45.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
780 bytes
89.91 KB

Good to know that we have test coverage for the title of styles/other plugins.

olli’s picture

Well, it fails otherwise with derivatives, which don't really define a title in the base implementation.

Not sure if this is true anymore after #27.

+++ b/core/modules/views/lib/Drupal/views/Annotation/ViewsPluginAnnotationBase.php
@@ -0,0 +1,41 @@
+  /**
+   * The module the plugin is defined by.
+   *
+   * @var string
+   */
+  public $module = 'views';

Should we move from 'module' to 'provider'?

olli’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/ViewsPluginManager.php
@@ -28,15 +26,17 @@ class ViewsPluginManager extends PluginManagerBase {
     $this->defaults += array(
       'parent' => 'parent',
@@ -44,6 +44,9 @@ public function __construct($type, \Traversable $namespaces) {

@@ -44,6 +44,9 @@ public function __construct($type, \Traversable $namespaces) {
       'module' => 'views',
       'register_theme' => TRUE,
     );

Are these defaults still needed?

dawehner’s picture

FileSize
648 bytes
89.86 KB

Should we move from 'module' to 'provider'?

Let's do that in a follow up, as this patch is already big enough, to be honest.

Are these defaults still needed?

Good point!

Status: Needs review » Needs work
Issue tags: -Plugin system, -Annotation

The last submitted patch, vdc-1969388-50.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#50: vdc-1969388-50.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Plugin system, +Annotation

The last submitted patch, vdc-1969388-50.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
89.91 KB

Let's just readd the old patch.

olli’s picture

#50: vdc-1969388-50.patch queued for re-testing.

Sorry for the noise, I dont see how those could be related to changes in the interdiff.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

I spoke to dawehner about the docs for properties on each annotation class etc.. Let's do this.

Berdir’s picture

Not completely sure, is this an API change or not? Meaning, will the old annotation classes stop working when this gets in or not?

dawehner’s picture

Issue tags: +API change

If you are strict this is an api change, that is right. Old views plugin will not work anymore.

dawehner’s picture

EclipseGC pointed out that because each annotation extends @Plugin, it will still work, if someone uses just @plugin. @ViewsPluginType is so just a nice way to document the available properties.

jibran’s picture

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

After #1911492: Views try to find Custom StylePlugin template in core/modules/views/templates went in it needs reroll. Sorry @dawehner you made that when critical and RTBC. ;)

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
89.73 KB

Rerolled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, vdc-1969388-61.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
993 bytes
90.7 KB

Ha!

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

damiankloip’s picture

+1 again, looks good

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

$id, $title and $short_title are used by all plugin types. Should they be on the base annotation class?
Also $no_ui is used by every plugin type but ViewsWizard so that might be a candidate as well, but probably a follow-up in that case.

dawehner’s picture

It is okay to move no_ui to the base class, but I reject to move title and short_title on there as well. The point of the issue is to be able
to have a place for a proper documentation of this keys.

Moving this to a baseclass will reduce the possibility to find it. Once we are at this point, we can drop it entirely because it is not needed for the actual functionality.

tstoeckler’s picture

Also @damiankloip just opened the follow-up we need here :-): #2070609: Remove obsolete "module" keys from Views plugin annotations

damiankloip’s picture

Yep, done :-). See #44/#45 for previous discussion about moving stuff to the based class.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Hmm... I don't actually think that this makes it more findable than in a base class, but I don't really care. Sorry, I had missed #44/#45 before.
Back to RTBC then.

damiankloip’s picture

I'm in the same boat as you on that, but yeah, I don't mind either.

dawehner’s picture

Issue tags: +VDC

TF.

alexpott’s picture

Title: Add dedicated annotations for Views plugins » Change notice: Add dedicated annotations for Views plugins
Project: Drupal core » Views (for Drupal 7)
Version: 8.x-dev » 8.x-3.x-dev
Component: views.module » Code
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Approved API change

Committed 55bd232 and pushed to 8.x. Thanks!

I don't think this needs a core change notice because views is new. But it falls into the views change process.

dawehner’s picture

Oh nice, thank you!!

tim.plunkett’s picture

Issue tags: -Needs reroll

Removing tags

tim.plunkett’s picture

Issue summary: View changes

asdf

Chris Matthews’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.7.x-dev
Component: Code » views.module
Assigned: dawehner » Unassigned
Issue summary: View changes

For more information as to why this issue was moved to the Drupal core project, please see issue #3030347: Plan to clean process issue queue

Chris Matthews’s picture

Project: Drupal core » Views (for Drupal 7)
Version: 8.7.x-dev » 7.x-3.x-dev
Component: views.module » Code
Status: Active » Closed (outdated)

Moving back to the contributed Views issue queue and closing as outdated per https://www.drupal.org/project/views/issues/3030347#comment-13023447