Problem/Motivation

In a similar vein as #2256355: Make block plugins usable outside the block entity context...

\Drupal\block\BlockForm is part of the block entity, and happens to always know which theme it will be used by.

Block plugins are themselves not able to know what theme they'll appear in.

The only block plugin relying on this is SystemBrandingBlock.

Amusingly demonstrated in https://www.youtube.com/watch?v=4JMMQW2Vnws

Proposed resolution

Do not put the theme in the $form_state
SystemBrandingBlock doesn't need to link directly to the theme, the appearance page is enough.

Remaining tasks

N/A

User interface changes

The system branding block form links to admin/appearance/settings still, but not admin/appearance/settings/{theme}

API changes

Block plugin forms no longer have access to the theme.

Release notes snippet

Block plugin forms no longer get a 'block_theme' key in form state. See the change record for more details.

CommentFileSizeAuthor
#25 2268787-block-25.patch2.9 KBtim.plunkett
#19 2268787-19.patch2.99 KBtim.plunkett
#19 interdiff.txt543 bytestim.plunkett
#17 2268787-17.patch2.75 KBtim.plunkett
#13 block-2268787-13.patch2.21 KBtim.plunkett
#8 block-2268787-8.patch2.35 KBcdracars
#1 block-2268787-1.patch2.46 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
block-2268787-1.patch2.46 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/block/lib/Drupal/block/BlockForm.php
--- a/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php

Why can't we at least try to fetch additional information from the current request and fallback in case we can't find out the theme?

tim.plunkett’s picture

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

Because its not about the theme used by the current request at all?
When you configure blocks for Bartik, you're still using the admin in Seven.

And we can't couple it to the route, since other usecases for configuring block plugins would have nothing in the route about the theme...

Also, it's just not worth it for a little bit of help text.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Oh, well I talk about the URL in which the theme should be encoded, isn't it?

tim.plunkett’s picture

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

No, the URL is built by the Block UI, which is not the same URL when a block is configured in the Page Manager UI.

If a block needed the theme during runtime, we could try declaring it as a context. But that's not the case here, it was just a helpful link in the admin UI that really wasn't that helpful.

cdracars’s picture

cdracars CreditAttribution: cdracars commented

1: block-2268787-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: block-2268787-1.patch, failed testing.

cdracars’s picture

cdracars CreditAttribution: cdracars commented
FileSize
block-2268787-8.patch2.35 KB

Patch Re-Rolled.

cdracars’s picture

cdracars CreditAttribution: cdracars commented
Status: Needs work » Needs review

tim.plunkett queued 8: block-2268787-8.patch for re-testing.

tim.plunkett queued 8: block-2268787-8.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: block-2268787-8.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
block-2268787-13.patch2.21 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned

mgifford queued 13: block-2268787-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: block-2268787-13.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2268787-17.patch2.75 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 17: 2268787-17.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff.txt543 bytes
2268787-19.patch2.99 KB

Forgot to hit paste after hitting cut :)

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs review » Needs work

Patch no longer applies.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
Issue tags: -Block plugins
FileSize
2268787-block-25.patch2.9 KB

Please tag "Needs reroll" next time, thanks!

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

re @dawehner's comment in #2

Why can't we at least try to fetch additional information from the current request and fallback in case we can't find out the theme?

and @tim.plunkett's response in #3

Because its not about the theme used by the current request at all?
When you configure blocks for Bartik, you're still using the admin in Seven.

Should we make a follow up to add the link to \Drupal\settings_tray\Form\SystemBrandingOffCanvasForm::buildConfigurationForm?
Because when using the 'settings_tray' plugin form then the current request's theme is the theme they would want to change the logo for.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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.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.

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.

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Contributed project blocker
Related issues: +#3119245: Some blocks require a block_theme value

Patch still applies.

The same error occurs in Block field:
#3119245: Some blocks require a block_theme value.

Patch in #25 fixes the problem.

  • catch committed 62430b7 on 9.3.x 
    Issue #2268787 by tim.plunkett: Block plugin forms should not rely on...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Considered asking for test coverage for this, but while it's possible to end up with a system branding block bug in contrib, this feels like just refactoring form structure in core itself and existing test coverage should suffice.

Also agreed that linking to the appearance page is enough here.

Committed 62430b7 and pushed to 9.3.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Are we concerned about the modules in contrib that rely on this? See http://grep.xnddx.ru/search?text=%24form_state-%3Eget%28%27block_theme%2... - they all have Drupal 9 compatibility.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Fixed » Needs work
Issue tags: +Needs change record

I don't think we should revert due to that, form structure is internal according to the bc policy, but let's at least add a change record.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes
Status: Needs work » Fixed
Issue tags: -Needs change record +9.3.0 release notes

Change record and release notes snippet added.

Status: Fixed » Closed (fixed)

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