Problem/Motivation

\Drupal\Core\Theme\ThemeNegotiator::determineActiveTheme() only checks for NULL so \Drupal\user\Theme\AdminNegotiator::determineActiveTheme() should return NULL when the admin theme is empty — i.e. the admin theme should be the default theme ie. should fallback to \Drupal\Core\Theme\DefaultNegotiator

Proposed resolution

Detect empty string and return NULL instead.

Atm you can also get "0" as a value but that's being taken care of in #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/A

CommentFileSizeAuthor
#7 interdiff.txt514 byteslauriii
#7 3084078-7.patch2.2 KBlauriii
#7 3084078-7-test-only.patch1.62 KBlauriii
#3 3084078-2.patch2.2 KBalexpott
#3 3084078-2.test-only.patch1.62 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott credited Jaesin.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3084078-2.test-only.patch1.62 KB
new 3084078-2.patch2.2 KB

The last submitted patch, 3: 3084078-2.test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 3084078-2.patch, failed testing. View results

Jaesin’s picture

Jaesin commented 
+++ b/core/modules/user/tests/src/Unit/Theme/AdminNegotiatorTest.php
@@ -0,0 +1,44 @@
+namespace Drupal\Tests\user\Unit\Menu;

This should be: namespace Drupal\Tests\user\Unit\Theme.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3084078-7-test-only.patch1.62 KB
new 3084078-7.patch2.2 KB
new interdiff.txt514 bytes

The last submitted patch, 7: 3084078-7-test-only.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

This is clearly violating the interface which says the following:

The name of the theme, or NULL if other negotiators, like the configured default one, should be used instead.

The fix looks correct and the test coverage is sufficient as proved by #7. I uploaded the most recent patch but my changes were trivial. Moving to RTBC.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Looks good!

(Just fixing nits in the issue summary.)

The last submitted patch, 7: 3084078-7-test-only.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3084078-7.patch, failed testing. View results

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs work » Reviewed & tested by the community

Head breakage. Status reset

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3084078-7.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Reviewed & tested by the community

Unrelated random failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 3084078-7.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Reviewed & tested by the community

As mentioned in comment #16.

The last submitted patch, 7: 3084078-7-test-only.patch, failed testing. View results

The last submitted patch, 7: 3084078-7-test-only.patch, failed testing. View results

The last submitted patch, 7: 3084078-7-test-only.patch, failed testing. View results

  • catch committed be4177a on 9.1.x 
    Issue #3084078 by lauriii, alexpott, Jaesin: AdminNegotiator::...

  • catch committed a6e1bd1 on 9.0.x 
    Issue #3084078 by lauriii, alexpott, Jaesin: AdminNegotiator::...

  • catch committed b29c168 on 8.9.x 
    Issue #3084078 by lauriii, alexpott, Jaesin: AdminNegotiator::...
catch’s picture

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

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

Status: Fixed » Closed (fixed)

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