Problem/Motivation

In #3073872: Allow for Layout Builder sections to be given administrative labels we added administrative labels, which added a new default configuration 'label' to the LayoutDefault class. However, layouts may choose to use a different class that might not have a label configuration, which results in an notice when trying to remove a section.

Notice: Undefined index: label in Drupal\layout_builder\Form\RemoveSectionForm->getQuestion() (line 28 of /var/www/html/web/core/modules/layout_builder/src/Form/RemoveSectionForm.php)
  public function getQuestion() {
    $configuration = $this->sectionStorage->getSection($this->delta)->getLayoutSettings();
    if ($configuration['label']) {
      return $this->t('Are you sure you want to remove @section?', ['@section' => $configuration['label']]);
    }

We're already being defensive in other places, such as in Element/LayoutBuilder.php:

$section_label = !empty($layout_settings['label']) ? $layout_settings['label'] : $this->t('Section @section', ['@section' => $delta + 1]);

So we should be here as well!

Steps to reproduce

Proposed resolution

Ensure we check that a label key actually exists in the configuration in RemoveSectionForm.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

johnwebdev created an issue. See original summary.

johnwebdev’s picture

Status: Active » Needs review
FileSize
787 bytes
adhershmnair’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed and applied the Patch #2. Patch applied successfully and seems the undefined index notice fixed.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Should we add test coverage for this? (not a rhetorical question, if it's a prohibitive amount of work then maybe not).

Could also use a comment explaining why we're being defensive.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

raman.b’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
3.82 KB
3.21 KB

Adding some test coverage to demonstrate the reported bug

The last submitted patch, 6: 3167901-6-test-only.patch, failed testing. View results

johnwebdev’s picture

Status: Needs review » Reviewed & tested by the community
Spokje’s picture

Re-uploading 3167901-6.patch so this one gets re-rested every 2 days.
Currently the test-only patch 3167901-6-test-only.patch is re-tested every 2 days.

catch’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

  • catch committed 73d179a on 9.2.x
    Issue #3167901 by raman.b, johnwebdev, Spokje: RemoveSectionForm assumes...

  • catch committed e83a7c8 on 9.1.x
    Issue #3167901 by raman.b, johnwebdev, Spokje: RemoveSectionForm assumes...
catch’s picture

Status: Fixed » Closed (fixed)

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