Deprecated function: strnatcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in LayoutPluginManager

Created on 9 October 2023, 5 months ago
Updated 25 February 2024, 7 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

Add missing annotations to Drupal\layout_builder\Plugin\Layout\BlankLayout so that getCategory() and getLabel() return string|\Drupal\Core\StringTranslation\TranslatableMarkup as required, instead of NULL.

Call getFilteredDefinitions() in LayoutPluginManager::getLayoutOptions().

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 in getFilteredDefinitions(), and invoke just the first hook if it is not provided.

Remaining tasks

Decide whether to implement one of the other approaches in this issue or in a follow-up.

User interface changes

None, except for not showing the PHP error.

API changes

To be determined.

Data model changes

None

Release notes snippet

N/A

🐛 Bug report
Status

Needs review

Version

11.0 🔥

Component
Layout builder 

Last updated 41 minutes 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 5 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
    5 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 5 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 4 months ago
  • last update 4 months ago
    30,437 pass
  • 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦

    Patch in #8 with coding standards fixed.

  • Status changed to Needs work 4 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 4 months ago
  • 🇳🇱Netherlands ricovandevin

    Triggering tests...

  • Status changed to Needs work 4 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 4 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 4 months ago
    30,476 pass, 2 fail
  • last update 4 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
    4 months ago
    Total: 10248s
    #42802
  • Status changed to Needs work 4 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 3 months ago
    30,679 pass, 2 fail
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 3 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 3 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 3 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update 3 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & pgsql-14.1
    last update 3 months ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 3 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 about 2 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 about 2 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 about 1 month ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 month ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & sqlite-3.34
    last update about 1 month ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & pgsql-14.1
    last update about 1 month ago
    Patch Failed to Apply
  • Pipeline finished with Success
    about 1 month 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
    22 days ago
    Total: 533s
    #91728
  • Status changed to Needs review 22 days 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 15 days 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 13 days 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
    12 days ago
    Total: 573s
    #98792
  • Status changed to Needs review 12 days 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.

Production build https://api.contrib.social 0.61.6-2-g546bc20