Make $consumer optional in Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions()

Created on 30 May 2024, 8 months ago

Problem/Motivation

In 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed , we call Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions() from Drupal\Core\Layout\LayoutPluginManager::getLayoutOptions(). Since the latter method is in the Core namespace, not a module, there is no meaningful $consumer to pass, but we have to pass something since it is a required parameter.

Proposed resolution

Make $consumer optional.

In these lines, skip the second hook and the optional fourth parameter to alter() if $consumer is not provided:

    $hooks[] = "plugin_filter_{$type}";
    $hooks[] = "plugin_filter_{$type}__{$consumer}";
    $this->moduleHandler()->alter($hooks, $definitions, $extra, $consumer);
    $this->themeManager()->alter($hooks, $definitions, $extra, $consumer);

Remaining tasks

User interface changes

None

API changes

Drupal\Core\Layout\LayoutPluginManager::getLayoutOptions() can be called without providing the $consumer<code> parameter.

Data model changes

None

Release notes snippet

N/A

📌 Task
Status

Active

Version

11.0 🔥

Component
Plugin 

Last updated 1 day ago

Created by

🇺🇸United States benjifisher Boston area

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @benjifisher
  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 763s
    #297922
  • Pipeline finished with Success
    4 months ago
    Total: 314s
    #297984
  • Pipeline finished with Success
    4 months ago
    Total: 327s
    #298078
  • 🇺🇸United States benjifisher Boston area

    @annmarysruthy:

    Thanks for working on this issue. I left a few comments on the MR.

  • Pipeline finished with Success
    4 months ago
    Total: 457s
    #299551
  • 🇮🇳India annmarysruthy

    @benjifisher Kindly review

  • 🇺🇸United States smustgrave

    Most likely will need a change record also so tagging for such.

  • 🇺🇸United States benjifisher Boston area

    I added a draft change record (CR), so I am removing that issue tag: https://www.drupal.org/node/3481873 .

    @annmarysruthy, thanks for continuing to work on this issue.

    My first comment on the MR was this:

    Since this function implements a method from an interface, we should also update the interface: ...

    I thought about this some more while writing the CR, and I think I was wrong. From Signature compatibility rules in the PHP docs:

    A signature is compatible if it respects the variance rules, makes a mandatory parameter optional, adds only optional new parameters and doesn't restrict but only relaxes the visibility.

    So we are allowed to make the parameter optional in the trait without changing the interface. On the other hand, if we make the parameter optional in the interface, then we will break any contrib module that has a custom implementation of getFilteredDefinitions(). I cannot think of how we can warn module maintainers with a deprecation notice before causing their code to fail.

    I am leaving the status at NW.

  • 🇺🇸United States benjifisher Boston area

    The motivation for this issue was to find a better solution for 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed . It is tempting to do that as part of this issue: in Drupal\Core\Layout\LayoutPluginManager::getLayoutOptions(), replace the line

        $filtered_definitions = $this->getFilteredDefinitions($this->getType());
    

    with

        $filtered_definitions = $this->getFilteredDefinitions();
    

    Unfortunately, #3392572 was fixed in Drupal 10.3.0, and it is possible that some module has already implemented hook_plugin_filter_layout__layout_alter() instead of hook_plugin_filter_layout_alter(). So I do not think we can make that change without first deprecating that hook.

Production build 0.71.5 2024