Problem/Motivation

BlockBase doesn't store the context mapping on submitConfigurationForm, even though it adds the necessary elements in build configuration form.

Steps to reproduce

  1. Add a field_block on layout builder.
  2. Notice that the context_mapping has not been stored.

Proposed resolution

Add the context_mapping form value to $this->configuration in BlockBase class after submitting the configuration form.

Remaining tasks

  • Fix. Done
  • Tests. Done

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

NA

CommentFileSizeAuthor
#8 3251378-8.patch807 bytesameymudras

Issue fork drupal-3251378

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rlmumford created an issue. See original summary.

rlmumford opened merge request !1446

rlmumford opened merge request !1447

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Active » Needs review

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

Can you update your PR to point to 9.5 please

ameymudras’s picture

ameymudras commented
Status: Needs work » Needs review
StatusFileSize
new 3251378-8.patch807 bytes

Refactoring the above patch to 9.5.x so that we can proceed with the issue fix

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thank you for the patch.

This will also need a valid test.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vidorado made their first commit to this issue’s fork.

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

vidorado changed the visibility of the branch 3251378-8.9.x to hidden.

vidorado changed the visibility of the branch 3251378-block-plugins-dont to hidden.

vidorado opened merge request !10924

vidorado’s picture

vidorado commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

Added a Kernel test to test the new behavior.

smustgrave closed merge request !1447

smustgrave closed merge request !1446

smustgrave’s picture

smustgrave commented
Assigned: rlmumford » Unassigned
Status: Needs review » Needs work

Left comments on MR for consistency

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

vidorado’s picture

vidorado commented
Status: Needs work » Needs review

@smustgrave, I've applied your two suggestions and left a reply to your third MR comment of what I think @rlmumford pretended to do.

Thanks!

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Leaving the 1 thread open but rest of the feedback appears to be addressed

  • catch committed 1ec49896 on 11.x 
    Issue #3251378 by vidorado, rlmumford, ameymudras, smustgrave: Block...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I kind of wondered if we want functional test coverage for this to show what actually breaks (which I am not clear about to be honest), but the kernel test is pretty clear.

Committed/pushed to 11.x, thanks!

catch closed merge request !10924

Status: Fixed » Closed (fixed)

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

douggreen’s picture

douggreen commented

This should have a change record because now any overrides of BlockBase::submitConfigurationForm() require the void typehint.

acbramley’s picture

acbramley commented

Yeah just hit #27 when hunting down phpunit failures on 11.2

PHP Fatal error: Declaration of Drupal\entity_hierarchy_microsite\Plugin\MicrositePluginTrait::submitConfigurationForm(array &$form, Drupal\Core\Form\FormStateInterface $form_state) must be compatible with Drupal\Core\Block\BlockBase::submitConfigurationForm(array &$form, Drupal\Core\Form\FormStateInterface $form_state): void in /data/app/modules/contrib/entity_hierarchy/modules/entity_hierarchy_microsite/src/Plugin/MicrositePluginTrait.php on line 90
harlor’s picture

harlor commented

Example in contrib: https://www.drupal.org/project/spa/issues/3526733#comment-16203161 (comment #8)

I'm wondering if we should specify the return type when there is no return type on the interface in this case PluginFormInterface yet.

acbramley’s picture

acbramley commented

@harlor see #3537720: [regression] Remove return type from BlockBase::submitConfigurationForm(), we are reverting the return type.