Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#9 | 3167901-6.patch | 3.82 KB | Spokje |
#6 | interdiff_2-6.txt | 3.21 KB | raman.b |
#6 | 3167901-6.patch | 3.82 KB | raman.b |
#6 | 3167901-6-test-only.patch | 2.96 KB | raman.b |
#2 | 3167901-2.patch | 787 bytes | johnwebdev |
Comments
Comment #2
johnwebdev CreditAttribution: johnwebdev commentedComment #3
adhershmnair CreditAttribution: adhershmnair commentedI have reviewed and applied the Patch #2. Patch applied successfully and seems the undefined index notice fixed.
Comment #4
catchShould 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.
Comment #6
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAdding some test coverage to demonstrate the reported bug
Comment #8
johnwebdev CreditAttribution: johnwebdev commentedComment #9
SpokjeRe-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.Comment #10
catchCommitted/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!
Comment #13
catch