- Issue created by @benjifisher
- First commit to issue fork.
- 🇺🇸United States benjifisher Boston area
@annmarysruthy:
Thanks for working on this issue. I left a few comments on the MR.
- 🇺🇸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 ofhook_plugin_filter_layout_alter()
. So I do not think we can make that change without first deprecating that hook.