Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts

Created on 9 October 2023, 8 months ago
Updated 30 May 2024, 2 days ago

Problem/Motivation

Deprecated function: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php).

appears on pages, where layouts are being listed in a select which uses $this->layoutPluginManager->getLayoutOptions() to get its options.

Example:

$form['field_layouts']['field_layout'] = [
      '#type' => 'select',
      '#title' => $this->t('Select a layout'),
      '#options' => $this->layoutPluginManager->getLayoutOptions(),
      '#default_value' => $layout_plugin->getPluginId(),
      '#ajax' => [
        'callback' => '::settingsAjax',
        'wrapper' => 'field-layout-settings-wrapper',
        'trigger_as' => ['name' => 'field_layout_change'],
      ],
    ];

(FieldLayoutEntityDisplayFormTrait.php)

layout_builder's layout_builder_blank aka BlankLayout neither has a label nor a category defined in its annotation:

/**
 * Provides a layout plugin that produces no output.
 *
 * @see \Drupal\layout_builder\Field\LayoutSectionItemList::removeSection()
 * @see \Drupal\layout_builder\SectionListTrait::addBlankSection()
 * @see \Drupal\layout_builder\SectionListTrait::hasBlankSection()
 *
 * @internal
 *   This layout plugin is intended for internal use by Layout Builder only.
 *
 * @Layout(
 *   id = "layout_builder_blank",
 * )
 */
class BlankLayout extends LayoutDefault {

it should be sorted out by

/**
 * Implements hook_plugin_filter_TYPE_alter().
 */
function layout_builder_plugin_filter_layout_alter(array &$definitions, array $extra, $consumer) {
  // Hide the blank layout plugin from listings.
  unset($definitions['layout_builder_blank']);
}

(layout_builder.module)

but that hook never gets called!

So the layout_builder_blank layout is being passed through without label or category, but these methods rely on these values to be present. They are being called indirectly by $this->layoutPluginManager->getLayoutOptions()

  /**
   * {@inheritdoc}
   *
   * @return \Drupal\Core\Layout\LayoutDefinition[]
   */
  public function getSortedDefinitions(array $definitions = NULL, $label_key = 'label') {
    // Sort the plugins first by category, then by label.
    $definitions = $definitions ?? $this->getDefinitions();
    uasort($definitions, function (LayoutDefinition $a, LayoutDefinition $b) {
      if ($a->getCategory() != $b->getCategory()) {
        return strnatcasecmp($a->getCategory(), $b->getCategory());
      }
      return strnatcasecmp($a->getLabel(), $b->getLabel());
    });
    return $definitions;
  }

  /**
   * {@inheritdoc}
   *
   * @return \Drupal\Core\Layout\LayoutDefinition[][]
   */
  public function getGroupedDefinitions(array $definitions = NULL, $label_key = 'label') {
    $definitions = $this->getSortedDefinitions($definitions ?? $this->getDefinitions(), $label_key);
    $grouped_definitions = [];
    foreach ($definitions as $id => $definition) {
      $grouped_definitions[(string) $definition->getCategory()][$id] = $definition;
    }
    return $grouped_definitions;
  }

  /**
   * {@inheritdoc}
   */
  public function getLayoutOptions() {
    $layout_options = [];
    foreach ($this->getGroupedDefinitions() as $category => $layout_definitions) {
      foreach ($layout_definitions as $name => $layout_definition) {
        $layout_options[$category][$name] = $layout_definition->getLabel();
      }
    }
    return $layout_options;
  }

This is why the error appears.

This deprecation warning didn't appear in older versions of PHP, so the bug wasn't visible.

TL;DR:

/**
 * Implements hook_plugin_filter_TYPE_alter().
 */
function layout_builder_plugin_filter_layout_alter(array &$definitions, array $extra, $consumer) {
  // Hide the blank layout plugin from listings.
  unset($definitions['layout_builder_blank']);
}

(layout_builder.module)
is not called, so the layout_builder_blank layout, which neither has a label nor a description is not being sorted out, which leads to this error!

Steps to reproduce

  1. Install Drupal with the Umami demo profile.
  2. Enable the field_layout module.
  3. Log in as an administrator.
  4. Visit /en/admin/structure/types/manage/article/form-display.

Alternatively, install Drupal with the Standard profile, then enable the field_layout and layout_builder modules.

Further analysis

It seems as like the LayoutPluginManager (DI-injected by $container->get('plugin.manager.core.layout'),) is entirely missing FilteredPluginManagerTrait, so that its alterations are never being called:

/**
   * Implements \Drupal\Core\Plugin\FilteredPluginManagerInterface::getFilteredDefinitions().
   */
  public function getFilteredDefinitions($consumer, $contexts = NULL, array $extra = []) {
    if (!is_null($contexts)) {
      $definitions = $this->getDefinitionsForContexts($contexts);
    }
    else {
      $definitions = $this->getDefinitions();
    }

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

meanwhile, they are being called in Layout builder (e.g. admin/structure/types/manage/page/display/default/layout) when adding a section.
So maybe layout_builder is doing something different or special?
If layout_builder is not installed, everything is fine!

Proposed resolution

  1. Add missing attributes to Drupal\layout_builder\Plugin\Layout\BlankLayout so that getCategory() and getLabel() return string|\Drupal\Core\StringTranslation\TranslatableMarkup as required, instead of NULL.
  2. Call getFilteredDefinitions() in LayoutPluginManager::getLayoutOptions().
  3. Add test coverage for (2).

Either (1) or (2) by itself is enough to avoid the error. If we implement (1) but not (2), then it leads to a regression: the blank layout is listed, when it currently is not. See Comment #50. If we implement (2) but not (1), then we are hiding the error instead of fixing it.

For (3), mock Drupal::service('theme.manager'), since that is called in FilteredPluginManagerTrait::themeManager(). Then call getLayoutOptions() and confirm that the module and theme alter() methods are called.

Remaining tasks

The decision is made: 📌 Make $consumer optional in Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions() Active is the follow-up issue.

User interface changes

None, except for not showing the PHP error.

API changes

Drupal\Core\Layout\LayoutPluginManager::getLayoutOptions()

Data model changes

respects hook_plugin_filter_alter(), so modules and themes can alter the list of layouts.

Release notes snippet

N/A

🐛 Bug report
Status

RTBC

Version

11.0 🔥

Component
Layout builder 

Last updated about 10 hours ago

Created by

🇩🇪Germany Anybody Porta Westfalica

Live updates comments and jobs are added and updated live.
  • PHP 8.2

    The issue particularly affects sites running on PHP version 8.2.0 or later.

  • PHP 8.1

    The issue particularly affects sites running on PHP version 8.1.0 or later.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Anybody
  • 🇩🇪Germany Anybody Porta Westfalica
  • last update 8 months ago
    30,382 pass
  • @anybody opened merge request.
  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇩🇪Germany Anybody Porta Westfalica

    The root cause for this error is, that label and category are empty, but expected to be a string.
    I had a look what this is caused by and it's:
    layout_builder_blank provided by layout_builder module.

    It happens when calling LayoutPluginManager::getLayoutOptions() for example to get the sorted options for a layout selection select (#options).

    Perhaps we should throw an exception, if no label and category are given by a layout.yml?

    Like the existing InvalidPluginDefinitionException here:

    /**
       * {@inheritdoc}
       */
      public function processDefinition(&$definition, $plugin_id) {
        parent::processDefinition($definition, $plugin_id);
    
        if (!$definition instanceof LayoutDefinition) {
          throw new InvalidPluginDefinitionException($plugin_id, sprintf('The "%s" layout definition must extend %s', $plugin_id, LayoutDefinition::class));
        }
    
  • 🇩🇪Germany Anybody Porta Westfalica

    Here's a screenshot of the layout definition that's being sorted:

    Any ideas?

  • Pipeline finished with Success
    8 months ago
    Total: 955s
    #28289
  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇬🇧United Kingdom sabrina.liman

    Patch for Deprecated function: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php). error

  • last update 8 months ago
    30,397 pass
  • 🇺🇸United States webdrips

    #8 fixed the deprecated code error for me on the Paragraphs manage form display, although I don't see any empty layout settings except the default one that shows up after installing layout builder (and field layout?):

    <option value="layout_builder_blank"></option>

    But then I didn't get that same deprecated function error on another more vanilla D10 instance, so I'm not sure what's going on.

  • Status changed to Needs review 7 months ago
  • last update 7 months ago
    30,437 pass
  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    Patch in #8 with coding standards fixed.

  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    Believe #6 was going the right track. Think there is a bug where label/category are empty. Think the fix should be there.

  • 🇩🇪Germany Anybody Porta Westfalica

    #8 and #10 are nice quickfixed but shouldn't the blank layout better get sorted out generally for

      public function getLayoutOptions()
    

    ?

    Can the subsystem maintainer give an answer perhaps?

    Indeed I think 🐛 strnatcasecmp(): Passing null to parameter #1 ($string) of type string is deprecated Closed: duplicate is a duplicate and here's a contrib case where a workaround is already in place that I think shouldn't be needed: 🐛 layout_builder_blank must not be disabled Fixed

    The following class should either implement all Plugin requirements like label and group or be removed before such calls, shouldn't it?

    <?php
    
    namespace Drupal\layout_builder\Plugin\Layout;
    
    use Drupal\Core\Layout\LayoutDefault;
    
    /**
     * Provides a layout plugin that produces no output.
     *
     * @see \Drupal\layout_builder\Field\LayoutSectionItemList::removeSection()
     * @see \Drupal\layout_builder\SectionListTrait::addBlankSection()
     * @see \Drupal\layout_builder\SectionListTrait::hasBlankSection()
     *
     * @internal
     *   This layout plugin is intended for internal use by Layout Builder only.
     *
     * @Layout(
     *   id = "layout_builder_blank",
     * )
     */
    class BlankLayout extends LayoutDefault {
    
      /**
       * {@inheritdoc}
       */
      public function build(array $regions) {
        // Return no output.
        return [];
      }
    
    }
    
  • 🇩🇪Germany Anybody Porta Westfalica

    I updated the issue summary, so that the steps and root cause should be clearer now.

  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇩🇪Germany Anybody Porta Westfalica
  • 🇳🇱Netherlands ricovandevin

    It seems as like the LayoutPluginManager (DI-injected by $container->get('plugin.manager.core.layout'),) is entirely missing FilteredPluginManagerTrait, so that its alterations are never being called:

    This is not entirely true. The plugin manager uses FilteredPluginManagerTrait but the getSortedDefinitions() method that is indirectly called from getLayoutOptions() does not use the getFilteredDefinitions() method to retrieve the layout definitions.

    I will create a MR with a fix for this. But I'm not sure about unintended side effects so input from the subsystem maintainer(s) is still appreciated.

  • 🇩🇪Germany Anybody Porta Westfalica

    @ricovandevin thank you!

    This is not entirely true. The plugin manager uses FilteredPluginManagerTrait but the getSortedDefinitions() method that is indirectly called from getLayoutOptions() does not use the getFilteredDefinitions() method to retrieve the layout definitions.

    Absolutely. That was just an idea what kind of issue might cause this. Technically it was definitely not the correct final anwser, just a pointer into that direction ;)

    Thanks for your help and clarification! Let's see what the tests say.

  • 🇳🇱Netherlands ricovandevin

    just a pointer into that direction ;)

    @Anybody It absolutely pointer me in the direction of getFilteredDefinitions()! :-)

    Apparently I cannot edit the MR. So hereby a patch that makes getSortedDefinitions() use the getFilteredDefinitions() method.

  • Status changed to Needs review 7 months ago
  • 🇳🇱Netherlands ricovandevin

    Triggering tests...

  • Status changed to Needs work 7 months ago
  • 🇳🇱Netherlands ricovandevin

    I was a bit too fast on this. The patch in #22 is not solving the issue. Must have missed a cache rebuild when testing this earlier.

  • 🇩🇪Germany Anybody Porta Westfalica

    @ricovandevin just create a separate MR, I think?

  • Status changed to Needs review 7 months ago
  • 🇳🇱Netherlands ricovandevin

    Updated the MR. Had to "get push access" first.

    There are two lines in which definitions are loaded. Replaced both by the filtered version. Patch attached for use with Composer.

  • last update 7 months ago
    30,476 pass, 2 fail
  • last update 7 months ago
    30,469 pass, 2 fail
  • 🇩🇪Germany Anybody Porta Westfalica

    Thanks @ricovandevin that looks promising, let's wait what the tests say. By the way I think to prove this, we should add a simple test which calls $layoutPluginManager->getLayoutOptions() and ensures only the expected layouts are included?

  • Pipeline finished with Failed
    7 months ago
    Total: 10248s
    #42802
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

    Agree on the tests but #26 also had an existing test failure.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 6 months ago
    30,679 pass, 2 fail
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 6 months ago
    30,680 pass, 2 fail
  • 🇮🇹Italy FiNeX

    Thanks for the patch. It works on D10.1

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 6 months ago
    29,715 pass, 2 fail
  • 🇺🇸United States droath

    The patch didn't work to resolve the error. I ended up trying a different approach, which seems to have resolved the issue.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & sqlite-3.34
    last update 6 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 6 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & pgsql-14.1
    last update 6 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 6 months ago
    Custom Commands Failed
  • 🇬🇧United Kingdom rossb89

    Can you elaborate on why the patch didn't work to resolve the error? I just tested it locally on 10.1.x and the error was resolved. Is it not working in 11.x then?

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 5 months ago
    Custom Commands Failed
  • 🇪🇸Spain candelas

    #26 works in Drupal 10.2.1 with php 8.1.27

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 5 months ago
    30,623 pass, 15 fail
  • 🇺🇸United States jayhuskins

    Micro-nit fix for #30: changing "null" to "NULL"

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 4 months ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 4 months ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & sqlite-3.34
    last update 4 months ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & pgsql-14.1
    last update 4 months ago
    Patch Failed to Apply
  • Pipeline finished with Success
    4 months ago
    Total: 643s
    #79617
  • Same problem in drupal 10.2.2 comment #33 was very useful to me

  • 🇵🇹Portugal saidatom Lisbon

    Reroll #30

  • 🇺🇸United States benjifisher Boston area

    @Anybody:

    Thanks for the detailed analysis in the issue summary. I agree that casting to string or using the null-coalesce operator would be fixing the symptom instead of the root cause.

    The problem with using getFilteredDefinitions() in LayoutPluginManager is that you are specifying $consumer that works for this case, but that Core class can be used anywhere in Drupal. When the block module calls getSortedDefinitions(), why should it invoke the hook from the layout_builder module? Related: when I search for getFilteredDefinitions(), I find that it is called in a few placed in the layout_builder module.

    I think you missed one point: LayoutDefinition::getCategory() declares its return type as string|\Drupal\Core\StringTranslation\TranslatableMarkup. It is not allowed to return null. So adding a category to the BlankLayout plugin annotations is the correct fix.

    I will prepare a merge request that does that. I will also add any other annotations that seem to be required, and make the @param comment for getSortedDefinitions() more specific.

  • Merge request !6533Add required plugin annotations → (Open) created by benjifisher
  • Pipeline finished with Success
    4 months ago
    Total: 533s
    #91728
  • Status changed to Needs review 4 months ago
  • 🇺🇸United States benjifisher Boston area

    On second thought, the @param comment for getSortedDefinitions() comes from the interface, so it is not so easy to make it more specific. Maybe we should, but not for just this plugin type.

    I added annotations for label and category. I considered adding a description, since getDescription() has the same return type declaration, but the annotation class specifically says that $description is optional. And then there is $templatePath. (shrug)

    I am updating the steps to reproduce and the proposed resolution in the issue summary.

    I am also attaching a patch for convenience.

    I am removing the "Needs tests" issue tag. I do not think we need to test this.

    BTW, @longwave commented in #3316811-8: strnatcasecmp(): Passing null to parameter #1 ($string) of type string is deprecated :

    It appears there is a plugin with a NULL label somewhere, it would be good to know exactly which plugin that is, and whether it is a core issue, or contrib or custom code.

  • 🇺🇸United States benjifisher Boston area

    In #3315678-27: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated , @tawellman posted a stack trace that showed the (core, experimental) field_layout module. Using that, I can reproduce the error using just Drupal core. I am updating the steps to reproduce in the issue summary here, closing the other issue as a duplicate, and giving credit to @tawellman on this issue.

    It might be appropriate to give credit to some of the others who commented on that issue.

  • 🇺🇸United States SocialNicheGuru

    Reading through the patches it is unclear which solution is recommended. The patches seem quite different for example #40 is different from those that precede it and the MR. Which MR is the one to use?

  • 🇺🇸United States smustgrave

    smustgrave changed the visibility of the branch sorted-definitions-safety to hidden.

  • 🇺🇸United States smustgrave

    smustgrave changed the visibility of the branch 11.x to hidden.

  • 🇺🇸United States smustgrave

    smustgrave changed the visibility of the branch 3392572-deprecated-function-strnatcasecmp to hidden.

  • Status changed to RTBC 4 months ago
  • 🇺🇸United States smustgrave

    Looking at the MR and the proposal to add missing annotation seems straightforward.

    Not sure this needs a follow up to catch these later? Not tagging as I'm not sure.

  • 🇬🇧United Kingdom longwave UK

    📌 Convert Layout plugin discovery to attributes Active will change this anyway, so I suggest we make the label and category properties required over there.

  • Status changed to Needs work 3 months ago
  • 🇬🇧United Kingdom longwave UK

    If we believe layout_builder_plugin_filter_layout_alter() is not being called I think we at least need a followup to investigate that.

  • Pipeline finished with Success
    3 months ago
    Total: 573s
    #98792
  • Status changed to Needs review 3 months ago
  • 🇺🇸United States benjifisher Boston area

    In further testing, I noticed that I can reproduce the problem using the Standard profile if I enable the field_layout and layout_builder modules.

    I also noticed what @Anybody meant in the issue summary:

    This should also solve the side-effect of layout_builder_blank appearing in selects!

    With 11.x, I do not see that option, but if I fix the plugin annotations and visit /admin/structure/types/manage/article/form-display, then layout_builder_blank appears in the Layout settings form near the bottom of the page. I think this counts as a regression, which means we cannot defer it to a follow-up issue. I am reverting the issue title to what it was before Comment #47 and removing the tag for a follow-up. While I am at it, I am changing the issue priority.

    I already updated MR 6533. I think this is the least disruptive fix: call getFilteredDefinitions() in LayoutPluginManager::getLayoutOptions(). The only problem is that we do not know which $consumer to pass as the first argument, so I just use $this->getType(). This ends up invoking both hook_plugin_filter_layout_alter and hook_plugin_filter_layout__layout_alter.

    We should consider other approaches:

    1. Add an optional parameter to LayoutPluginManager::getLayoutOptions() and pass it to getFilteredDefinitions(), using $this->getType() as a fallback.
    2. Make $consumer optional, and invoke just the first hook if it is not provided.

    I will mention these options in the issue summary. Maybe the safest approach is to use the current fix and then implement (1) in a follow-up.

  • 🇨🇦Canada smulvih2 Canada 🍁

    #10 works for my on core 10.1.8, but also using on a few other projects. Need this patch in order to create new panels page.

  • 🇺🇸United States smustgrave

    So this still in review to decided on the approach and what can be a follow up, least that's how I read the last part of #50.

  • 🇪🇸Spain lapurddrupal

    Dear promoters,
    I have incorporated patch #10. It works for. I would like to take this opportunity to thank all the staff from the bottom of my heart for their work here in this community.

  • Status changed to RTBC 3 months ago
  • 🇺🇸United States smustgrave

    Opened 📌 Explore additional approaches to handle missing ID or Category in layout plugin Active to explore additional approaches. But agree with @benjifisher that the current MR does fix the error here.

  • Status changed to Needs work 3 months ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    I think we should be adding test coverage of the changes to getLayoutOptions().

  • 🇺🇸United States benjifisher Boston area

    @alexpott:

    Thanks for the review!

    I do not know the plugin system as well as I would like. I guess the @ingroup plugin_translatable in Drupal\Core\Layout\Annotation\Layout is what tells us that the annotation should be translatable? I could not find the documentation for that. But I added a commit with what I think you meant.

    I started to add a test to Drupal\Tests\Core\Layout\LayoutPluginManagerTest, but as soon as I call getLayoutOptions() from that test, I get a ServiceNotFoundException for the theme.manager service. This comes from FilteredPluginManagerTrait:

          protected function themeManager() {
            if (property_exists($this, 'themeManager') && $this->themeManager instanceof ThemeManagerInterface) {
              return $this->themeManager;
            }
    
            return \Drupal::service('theme.manager');
          }
    

    The problem is that Drupal\Core\Layout\LayoutPluginManager does not have a $themeManager property. It does have a $themeHandler property, which it uses, but just to add some confusion the constructor has the misleading comment

           * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
           *   The theme handler to invoke the alter hook with.
    

    I think we should inject the theme.manager service into LayoutPluginManager. That seems outside the scope of this bug-fix issue. I am not sure how best to manage the scope, but here are some options:

    • In this issue, just add the annotations. Accept the regression ("Blank layout" appears in the list) for now, and fix it in a follow-up issue after injecting the theme.manager service.
    • Add test coverage as part of this module, but use a kernel test instead of a unit test. Can kernel tests go in core/tests/Drupal/Tests/Core? If not, where do we put the test?
    • Postpone this issue on the one to inject the theme.manager service.
  • Status changed to Needs review 3 months ago
  • 🇺🇸United States benjifisher Boston area
  • 🇧🇷Brazil carolpettirossi Campinas - SP

    The code proposed in the MR did not solve the deprecated issues for me.

    In our case we moved the 'Block Description' outside of the Form Display of a bunch of block types.

    We didn't know that disabling Block Description would result in lots of Blocks with empty Admin Labels.

    As a result, when we click to "Create a new block" in Layout Builder, lots of "Deprecated" messages are being logged:

    Deprecated function: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in Drupal\Core\Block\BlockManager->Drupal\Core\Plugin\{closure}() (line 95 of /var/www/web/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php)
    #0 /var/www/web/core/includes/bootstrap.inc(164): _drupal_error_handler_real(8192, 'strnatcasecmp()...', '/var/www/web/co...', 95)
    #1 [internal function]: _drupal_error_handler(8192, 'strnatcasecmp()...', '/var/www/web/co...', 95)
    #2 /var/www/web/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php(95): strnatcasecmp('', NULL)
    #3 [internal function]: Drupal\Core\Block\BlockManager->Drupal\Core\Plugin\{closure}(Array, Array)
    #4 /var/www/web/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php(96): uasort(Array, Object(Closure))
    #5 /var/www/web/core/lib/Drupal/Core/Block/BlockManager.php(76): Drupal\Core\Block\BlockManager->traitGetSortedDefinitions(Array, 'admin_label')
    #6 /var/www/web/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php(105): Drupal\Core\Block\BlockManager->getSortedDefinitions(Array, 'label')
    #7 /var/www/web/core/modules/layout_builder/src/Controller/ChooseBlockController.php(181): Drupal\Core\Block\BlockManager->getGroupedDefinitions(Array)
    #8 /var/www/web/modules/contrib/layout_builder_restrictions/src/Controller/ChooseBlockController.php(42): Drupal\layout_builder\Controller\ChooseBlockController->inlineBlockList(Object(Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage), 0, 'blb_region_col_...')
    #9 [internal function]: Drupal\layout_builder_restrictions\Controller\ChooseBlockController->inlineBlockList(Object(Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage), '0', 'blb_region_col_...')
    #10 /var/www/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)
    #11 /var/www/web/core/lib/Drupal/Core/Render/Renderer.php(627): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
    #12 /var/www/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
    #13 /var/www/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
    #14 /var/www/vendor/symfony/http-kernel/HttpKernel.php(181): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
    #15 /var/www/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
    #16 /var/www/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #17 /var/www/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #18 /var/www/web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #19 /var/www/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\ContentLength->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #20 /var/www/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #21 /var/www/web/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #22 /var/www/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #23 /var/www/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #24 /var/www/web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #25 /var/www/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #26 /var/www/web/core/lib/Drupal/Core/DrupalKernel.php(704): Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
    #27 /var/www/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
    #28 {main}
    

    The patch suggested here: https://www.drupal.org/project/drupal/issues/3316811#comment-14796459 🐛 strnatcasecmp(): Passing null to parameter #1 ($string) of type string is deprecated Closed: duplicate does solve our issues.

  • Status changed to Needs work 2 months ago
  • 🇧🇷Brazil carolpettirossi Campinas - SP
  • Status changed to Needs review 2 months ago
  • 🇺🇸United States benjifisher Boston area

    @carolpettirosi:

    Your problem is different from the one in this issue: your stack trace shows that strnatcasecmp() was called in core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php. This issue deals with the same function called in core/lib/Drupal/Core/Layout/LayoutPluginManager.php.

    Just as we have been discussing on this issue, you should look at the stack trace and decide what is the right place to fix the error. It is easy to add ?? '' whenever you use strnatcasecmp(), but that may be hiding the real problem instead of fixing it.

    The function strnatcasecmp() is used many times in the codebase:

    $ grep --recursive --count strnatcasecmp core | grep -v :0$ | sort
    core/lib/Drupal/Component/Utility/SortArray.php:1
    core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:1
    core/lib/Drupal/Core/Datetime/Entity/DateFormat.php:1
    core/lib/Drupal/Core/Entity/EntityDisplayModeBase.php:1
    core/lib/Drupal/Core/Language/Language.php:1
    core/lib/Drupal/Core/Layout/LayoutPluginManager.php:2
    core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php:2
    core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:1
    core/lib/Drupal/Core/Test/TestDiscovery.php:2
    core/modules/config/src/Form/ConfigSingleExportForm.php:1
    core/modules/config/src/Form/ConfigSingleImportForm.php:1
    core/modules/config_translation/src/Controller/ConfigTranslationEntityListBuilder.php:1
    core/modules/config_translation/src/Controller/ConfigTranslationMapperList.php:1
    core/modules/filter/src/FilterPluginCollection.php:1
    core/modules/help/src/Plugin/HelpSection/HelpTopicSection.php:1
    core/modules/shortcut/src/Entity/Shortcut.php:1
    core/modules/system/src/Entity/Action.php:1
    core/modules/workspaces/src/WorkspaceRepository.php:1
    core/node_modules/@cspell/dict-cpp/dict/cpp-legacy.txt:1
    core/node_modules/@cspell/dict-cpp/dict/cpp.txt:1
    core/node_modules/@cspell/dict-php/dict/php.txt:1
    

    Let's not try to handle all of them in this issue.

  • 🇦🇺Australia pasan.gamage

    Tested on core 10.2.4 and works.
    Error message no longer displaying.
    Used patch created from MR https://git.drupalcode.org/project/drupal/-/merge_requests/6533.patch
    See attached.

  • 🇺🇸United States smethawee

    Hi @ pasan.gamage No luck for me at all. I used this patch #61. Any comments or how I will make it work? Thanks!

    Error message
    Deprecated function: strnatcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php).
    Deprecated function: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php).
    Deprecated function: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php).
    Deprecated function: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php).
    Deprecated function: strnatcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in Drupal\Core\Layout\LayoutPluginManager->Drupal\Core\Layout\{closure}() (line 204 of core/lib/Drupal/Core/Layout/LayoutPluginManager.php).

  • 🇺🇸United States benjifisher Boston area

    @smethawee:

    Did you clear caches after applying the patch?

    Do you have any custom or contrib modules that define layouts using PHP classes? The patch fixes core/modules/layout_builder/src/Plugin/Layout/BlankLayout.php, but you might have files like mymodule/src/Plugin/Layout/MyLayout.php with the same problem.

  • 🇺🇸United States smethawee

    Thank you for your asked @benjifisher

    Yes, I did drush cr after applied the path. and No, I don't have any custom or contrib modules hat define layouts using PHP classes.
    So I need to working on it to work out.

    Thanks!

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 2 months ago
    29,722 pass
  • Status changed to Postponed about 1 month ago
  • Per #17 📌 Convert Layout plugin discovery to attributes Active on 📌 Convert Layout plugin discovery to attributes Active , labels should be added to all existing layout plugin definitions that are missing them (including a new on in Navigation), and deprecation warning triggered for contrib/custom plugins missing labels (and categories, as well. Blocking this issue on 📌 Convert Layout plugin discovery to attributes Active for the Layout attribute class to get in.

  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    Is there a reason we don't quickly fix this with a change like in the merge request or #10? A more complicated change could be done in a follow-up. For now, we should do a simple, reliable change that fixes the deprecation message.

  • Status changed to Needs work 7 days ago
  • 🇺🇸United States benjifisher Boston area

    Since 📌 Convert Layout plugin discovery to attributes Active is Fixed, we can un-postpone this issue.

    It looks as though the MR has some conflicts that need to be resolved, so I am setting the status back to NW, not NR.

  • Status changed to Needs review 6 days ago
  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    Merge request has been rebased.

  • Pipeline finished with Failed
    6 days ago
    Total: 335s
    #182654
  • Status changed to Needs work 6 days ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    6 days ago
    Total: 180s
    #182773
  • Status changed to Needs review 5 days ago
  • 🇺🇸United States benjifisher Boston area

    @Liam Morland: I had a look at your commits. It looks as though you switched from attributes to annotations, reversing the change from 📌 Convert Layout plugin discovery to attributes Active . Let's stick with attributes.

    I updated the MR and added some test coverage, as requested in Comment #55. I hope it is enough: in a unit test, I confirm that the alter() methods from the module handler and the theme manager are called, with the expected arguments, during the execution of getLayoutOptions(). I could also add a kernel test that confirms the expected effect on the result of getLayoutOptions().

    I am also updating the Proposed resolution section of the issue summary.

  • Pipeline finished with Success
    5 days ago
    Total: 1722s
    #183535
  • Status changed to RTBC 4 days ago
  • 🇺🇸United States smustgrave

    Hiding patches for clarity.

    Added a super nitpicky :void return

    But appears the thread for adding test coverage has been addressed.

  • Pipeline finished with Success
    4 days ago
    Total: 510s
    #184197
  • 🇳🇿New Zealand quietone New Zealand

    I skimmed the issue summary and at first thought the proposed resolution was a set of options. It is really the implemented solution. I also confirmed the problem exits in 10.2 and 11.x. I read the comments and didn't find any unanswered questions.

    I looked at the MR and made one small suggestion.

    What this does need is a title update to something sensible for the git log. Maybe 'Get layout options only for filtered layout plugins"? I am not sure though.

  • 🇳🇿New Zealand quietone New Zealand

    Update credit

  • 🇺🇸United States benjifisher Boston area

    @quietone:

    Thanks to you and @smustgrave for the updates to the MR.

    The original title is just the error message we are fixing, Except for the "in LayoutPluginManager" at the end, it is an error message that comes up a lot (as some of the comments on this issue remind us). I agree that the title (and the commit message) should be more specific.

    It is a little tricky since this issue makes two changes: the first change fixes the error, and the second change fixes a regression caused by the first change. I am updating the title to mention both, and I am reusing some text from the doc block of the test I added.

    While testing this issue, I noticed that the experimental navigation module adds a layout option with a category but no label, so I added 🐛 Add missing 'label' key to navigation.layouts.yml Active .

    I added 📌 Make $consumer optional in Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions() Active as a follow-up issue. I am updating (shortening) the "Proposed resolution" section of the issue summary here: since this issue is RTBC, I assume for now that we agree that should be done in a follow-up issue, not as part of this one. For the record, here is the alternative I suggested, now also removed from the issue summary:

    Add an optional parameter to LayoutPluginManager::getLayoutOptions() and pass it to getFilteredDefinitions(), using $this->getType() as a fallback.

  • There was follow up work needed from [#3420983} to make label and category properties required (see #65 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts RTBC ). The thought was to do it in this issue, but if it's out of scope here, I think a new follow up needs to be created to address that.

    One thought is that perhaps category does not need to be required, but we can follow the approach in Drupal\Core\Plugin\CategorizingPluginManagerTrait\processDefinitionCategory(), where the fallback category is the provider name.

Production build 0.69.0 2024