Problem/Motivation

In #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms), the SubFormState class was introduced for BlockPlugin's build/validate/submit callbacks, and additionally $form['settings'] (the sub form array) started getting passed to the build/validate callbacks, but not to the submit callback!

That means that $form is different in these two callbacks for everyone writing a BlockPlugin:

($form is $form['settings'] here)
public function blockValidate($form, FormStateInterface $form_state) {

($form is the complete/parent form here)
public function blockSubmit($form, FormStateInterface $form_state) {

Proposed resolution

We should fix this so that the $form argument passed to these callbacks is consistent.

Remaining tasks

Review the patch.

User interface changes

None.

API changes

The sub form is now passed to BlockPlugin's submitForm method, instead of the complete/parent form.

Data model changes

None.

CommentFileSizeAuthor
#21 2948549-21.patch3.25 KBmegha_kundar
#13 2948549-block_subform-13-interdiff.txt1.75 KBtim.plunkett
#13 2948549-block_subform-13.patch3.25 KBtim.plunkett
#12 2948549-12.patch2.48 KBabh.ai
#5 2948549-5-test-only.patch1.61 KBsamuel.mortenson
#5 2948549-5.patch2.48 KBsamuel.mortenson
block-plugin-form-2537732-follow-up.patch890 bytessamuel.mortenson

Issue fork drupal-2948549

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

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented
Related issues: +#2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms)
idebr’s picture

idebr commented

According to Drupal\Core\Plugin\PluginFormInterface the $form is the complete plugin form. Should this change not be the other way around, so BlockForm::validateForm uses $form instead of $form['settings'] as the first argument?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Needs tests

Whatever you pass to the method will be called $form, yes. If you see the form() method in BlockForm, it passes an empty array to buildConfigurationForm() which becomes $form to the plugin.

This is the correct change, but a test is needed.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented
Issue tags: -Needs tests
StatusFileSize
new 2948549-5.patch2.48 KB
new 2948549-5-test-only.patch1.61 KB

Here's a simple test.

Status: Needs review » Needs work

The last submitted patch, 5: 2948549-5-test-only.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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.

abh.ai’s picture

abh.ai commented
StatusFileSize
new 2948549-12.patch2.48 KB

Re-rolled patch and fixed test case.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 2948549-block_subform-13.patch3.25 KB
new 2948549-block_subform-13-interdiff.txt1.75 KB

Fixed failing tests.

abh.ai’s picture

abh.ai commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs release note

This seems like a good bugfix, and I think it's fine for a minor release, but, we should have a change record since it's possible modules are relying on the existing behaviour.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

We need this fix also in Layout Builder, in Drupal\layout_builder\Form\ ConfigureBlockFormBase::submitForm()

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Yep I'd bet a lot of money that I copy/pasted that from BlockForm to here. Good catch

andypost’s picture

andypost
he/him
Primary language Russian
commented

It may also throw deprecation when settings key used to catch old expectations

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Parent issue: » #3214771: (Plan) Make Subforms full citizens

Consolidating subform related features and wtfs, as in IEF we're working on this.

megha_kundar’s picture

megha_kundar commented
StatusFileSize
new 2948549-21.patch3.25 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

I don't see any difference between #21 and #13, can you explain @Megha_kundar?

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.

super_romeo’s picture

super_romeo commented

Patch #13 Patch Failed to Apply

Does this patch still needed?

super_romeo opened merge request !2846

super_romeo’s picture

super_romeo commented
Status: Needs work » Needs review

Rerolled against D9.4.

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 Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Hiding the patches as the MR is the path forward for this ticket.

Verified the tests fail without the fix.

#16 appears to still be needed

Moving back to NW for that and the change record + release notes.

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.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

@super_romeo please edit the merge request so that it targets 11.x.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

I have updated the contents of the merge request so that they are on top of 11.x.

Tim Bozeman made their first commit to this issue’s fork.

harjappan singh’s picture

harjappan singh commented

Hi! Are there any updates, or when is it planning to be shipped?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.