Problem/Motivation

We can now document an array of objects of a certain type. Let's use that in views in as many places as possible.


...
  /**
   * @var \Drupal\foo\baz\Ba[]
   */
  protected $bas;
...

Proposed resolution

Places where we can use it:

  • \Drupal\views\ViewExecutable::$old_views
  • \Drupal\views\ViewExecutable::$parent_views
  • \Drupal\views\ViewExecutable::$field
  • \Drupal\views\ViewExecutable::$argument
  • \Drupal\views\ViewExecutable::$sort
  • \Drupal\views\ViewExecutable::$filter
  • \Drupal\views\ViewExecutable::$relationship
  • \Drupal\views\ViewExecutable::$header
  • \Drupal\views\ViewExecutable::$footer
  • \Drupal\views\ViewExecutable::$empty
  • \Drupal\views\Plugin\views\display\DisplayPluginBase::$handlers
  • \Drupal\views\Plugin\views\display\DisplayPluginBase::$plugins

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#11 drupal-2356183-11.patch3.63 KBmichaellenahan
#8 drupal-2356183-6.patch3.61 KBmichaellenahan
#5 drupal-2356183-5.patch3.6 KBmichaellenahan
#3 drupal-2356183-3.patch2.87 KBmichaellenahan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Assigned: Unassigned » michaellenahan
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented

To provide better type-checking in IDEs, we can explicitly say what kind of array it is in the comment.

Here's an example, $old_view is an array of ViewExecutable objects.

  /**
   * Used to store views that were previously running if we recurse.
   *
   * @var \Drupal\views\ViewExecutable[]
   */
  public $old_view = array();
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
FileSize
drupal-2356183-3.patch2.87 KB

Here's a patch, but I wasn't sure what to do with:

\Drupal\views\Plugin\views\display\DisplayPluginBase::$handlers
\Drupal\views\Plugin\views\display\DisplayPluginBase::$plugins

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/src/ViewExecutable.php
@@ -253,67 +253,49 @@ class ViewExecutable {
    *
-   * An array containing Drupal\views\Plugin\views\area\AreaPluginBase objects.
-   *
-   * @var array
+   * @var Drupal\views\Plugin\views\area\AreaPluginBase[]
    */
   public $footer;

Really cool!

DisplayPluginBase::$handlers could be \Drupal\views\Plugin\views\HandlerBase[]
and \Drupal\views\Plugin\views\PluginBase[]

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
FileSize
drupal-2356183-5.patch3.6 KB

OK. Makes sense :)

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Status: Active » Needs review
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Assigned: michaellenahan » Unassigned
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
FileSize
drupal-2356183-6.patch3.61 KB

Forgot a backslash before \Drupal in some places, for example:
\Drupal\views\Plugin\views\area\AreaPluginBase[]

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Awesome michael.

See you a bit more in the issue queue, if you like!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Needs review

This is mostly great!

One question though... Normally rather than using ClassName[] we would want to have InterfaceName[] for the variable type.

For instance, I think instead of \Drupal\views\Plugin\views\PluginBase[] it should maybe be ViewsPluginInterface?

See https://www.drupal.org/node/1354#types
"Use interface names if possible, or the most general possible class, in place of a specific class."

So, ... not sure if these are all the most general class or interface?

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
FileSize
drupal-2356183-11.patch3.63 KB

The only interfaces could find were: ViewsHandlerInterface, ViewsPluginInterface.
Otherwise, I *think* the classes are as general as they get.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Yeah this is pretty much it.

@jhodgon
Interfaces makes sense if you have a design which is decoupled, but in views this is not that fine yet.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 999b26c and pushed to 8.0.x. Thanks!

  • alexpott committed 999b26c on 8.0.x 
    Issue #2356183 by michaellenahan | dawehner: Use array of type X in...
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Yeah, great! I just wanted to make sure that where we *have* interfaces, we use them in the docs. +1 on the RTBC and commit. :) Thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@michaellenahan
In case you want to continue to help a bit, #2359703: Remove public visibility from pager variables on the ViewExecutable is a bit more advanced issue, in order to get you more addicted ;)

Status: Fixed » Closed (fixed)

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