Problem/Motivation

In #3252386: Use PHP attributes instead of doctrine annotations we added support for attribute based plugin discovery.
As part of that issue we converted block and action plugins.

This issue is to convert \Drupal\views\Annotation\ViewsPagerplugins to use Attributes.

To do that we need #3421008: Convert ViewsExposedForm plugin discovery to attributes first. This issue is postponed on that.

Proposed resolution

  1. Add a class to represent the new Attribute - Example
  2. Update the plugin manager constructor to include both the attribute and annotation class names - example
  3. Convert all plugins that use the annotation to use the new attribute - example

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3421004

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Title: [PP-1] Convert ViewsPager plugin discovery to attributes » Convert ViewsPager plugin discovery to attributes
Issue summary: View changes
Status: Postponed » Active
Related issues: -#3421006: Convert ViewsDisplayExtender plugin discovery to attributes
larowlan’s picture

Issue summary: View changes
larowlan’s picture

Title: Convert ViewsPager plugin discovery to attributes » [PP-1] Convert ViewsPager plugin discovery to attributes
Issue summary: View changes
Status: Active » Postponed
Related issues: +#3421008: Convert ViewsExposedForm plugin discovery to attributes
alexpott’s picture

Status: Postponed » Active
alexpott’s picture

Title: [PP-1] Convert ViewsPager plugin discovery to attributes » Convert ViewsPager plugin discovery to attributes

Ruturaj Chaubey made their first commit to this issue’s fork.

ruturaj chaubey’s picture

Status: Active » Needs review
Issue tags: +Needs tests
smustgrave’s picture

Status: Needs review » Needs work

Appears to have test failures

Why was it tagged for tests?

ruturaj chaubey’s picture

Issue tags: -Needs tests

sorlov made their first commit to this issue’s fork.

quietone made their first commit to this issue’s fork.

quietone’s picture

Status: Needs work » Needs review

In trying to figure out why the test was failing I notices that two of the plugins do not declare a theme. That means that 'register_theme' will be TRUE which makes no sense because there is no theme declared in the plugin. Therefore, this adds register_theme: FALSE, for those two plugins.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Great investigative work @quietone!

I'm seeing that fixed all the tests and searched the repo and all 4 instances of @ViewsPager have been replaced.

I made 1 small nitpicky change adding "class-" to the driver.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

$theme has the wrong default value - causing the problems with register theme.

alexpott’s picture

Here are the current definitions for none and some... let's not change register_theme. It'll be work once the attribute as a theme value set to NULL rather than an empty string. Which makes more sense anyways.

    "none" => [
      "parent" => "parent",
      "plugin_type" => "pager",
      "register_theme" => true,
      "title" => Drupal\Core\StringTranslation\TranslatableMarkup {#7635},
      "short_title" => "",
      "help" => Drupal\Core\StringTranslation\TranslatableMarkup {#7645},
      "id" => "none",
      "display_types" => [
        "basic",
      ],
      "class" => "Drupal\views\Plugin\views\pager\None",
      "provider" => "views",
    ],
    "some" => [
      "parent" => "parent",
      "plugin_type" => "pager",
      "register_theme" => true,
      "title" => Drupal\Core\StringTranslation\TranslatableMarkup {#7647},
      "short_title" => "",
      "help" => Drupal\Core\StringTranslation\TranslatableMarkup {#7648},
      "id" => "some",
      "display_types" => [
        "basic",
      ],
      "class" => "Drupal\views\Plugin\views\pager\Some",
      "provider" => "views",
    ],
quietone’s picture

Status: Needs work » Needs review

@alexpott, thanks for the changes. I thought I had changed $theme to NULL and tested before I went on holiday. I guess I was wrong about that. Maybe I should have tried again.

Changes made, tests are passing. So back to needs review.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

So " vs ' I don't think there is a standard for that right?

I see for example #[ViewsRow are using " but #[ViewsArgument used '

Most appear to be using " but if a follow up is needed to fix #[ViewsArgument then we should maybe make it inline too right?

#[ViewsArgument("node_type")]

vs

#[ViewsArgument(
  id: 'node_type',
)]

But changes here look good.

alexpott’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0eca710d8b to 11.x and 95bfc525bd to 10.3.x. Thanks!

diff --git a/core/modules/views/src/Attribute/ViewsPager.php b/core/modules/views/src/Attribute/ViewsPager.php
index 0c222c4e03..2a19944ced 100644
--- a/core/modules/views/src/Attribute/ViewsPager.php
+++ b/core/modules/views/src/Attribute/ViewsPager.php
@@ -20,14 +20,14 @@ class ViewsPager extends Plugin {
    *
    * @param string $id
    *   The plugin ID.
-   * @param \Drupal\Core\StringTranslation\TranslatableMarkup|null $title
+   * @param \Drupal\Core\StringTranslation\TranslatableMarkup $title
    *   The plugin title used in the views UI.
    * @param \Drupal\Core\StringTranslation\TranslatableMarkup|null $short_title
    *   (optional) The short title used in the views UI.
    * @param \Drupal\Core\StringTranslation\TranslatableMarkup|null $help
    *   (optional) A short help string; this is displayed in the views UI.
    * @param string|null $theme
-   *   The theme function used to render the pager's output.
+   *   (optional) The theme function used to render the pager's output.
    * @param string[]|null $display_types
    *   (optional) The types of the display this plugin can be used with.
    *   For example the Feed display defines the type 'feed', so only rss style

Fixed on commit.

  • alexpott committed 95bfc525 on 10.3.x
    Issue #3421004 by Ruturaj Chaubey, sorlov, quietone, smustgrave,...

  • alexpott committed 0eca710d on 11.x
    Issue #3421004 by Ruturaj Chaubey, sorlov, quietone, smustgrave,...

Status: Fixed » Closed (fixed)

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