Problem/Motivation

Because of the configuration system, there is no reason for ThemeHandler::setDefault() to exist.

Proposed resolution

Mark ::setDefault as deprecated before Drupal 9 and replace usages with a configuration change, like this example from a test:

$this->config('system.theme')->set('default', 'bartik')->save();

Remaining tasks

User interface changes

None

API changes

ThemeHandler::setDefault() will be deprecated.

CommentFileSizeAuthor
#57 interdiff.txt2.47 KBManuel Garcia
#57 deprecate-2635784-57.patch17.94 KBManuel Garcia
#55 interdiff.txt4.96 KBManuel Garcia
#55 deprecate-2635784-55.patch15.46 KBManuel Garcia
#51 deprecate-2635784-51.patch10.5 KBManuel Garcia
#50 deprecate-2635784-50.patch10.51 KBManuel Garcia
#41 setdefault-on-2635784-41.patch10.29 KBsidharthap
#39 setdefault-on-2635784-39.patch10.29 KBsidharthap
#36 setdefault-on-2635784-36.patch10.55 KBsidharthap
#25 setdefault_on-2635784.patch9.65 KBveerasekar89
#22 2635784-revert.patch9.41 KBcatch
#11 setdefault_on-2635784-11.patch9.39 KBgambry
#2 setdefault_on-2635784-2.patch9.32 KBcilefen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen created an issue. See original summary.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Active » Needs review
FileSize
setdefault_on-2635784-2.patch9.32 KB

Status: Needs review » Needs work

The last submitted patch, 2: setdefault_on-2635784-2.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
jordanpagewhite’s picture

jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commented

@cilefen Is there a reason that you're using $this->themeHandler in some methods and \Drupal::service('theme_handler') in others?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

@jordanpagewhite Yes.

Some of those test classes don't have the themeHandler as a member. If any do, and I used \Drupal, those should be changed.

Edited: I meant "classes".

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Got them all:) thanks @cilefen

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.1.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +needs backport to 8.0.x

Committed/pushed to 8.1.x, thanks!

Given this is a test-only change, I don't see why it couldn't go into 8.0.x too, but assuming this was put at 8.1.x for a reason. Moving branch and tagging for now.

  • catch committed 314de72 on 8.1.x 
    Issue #2635784 by cilefen: setDefault() on ThemeHandler is not used in...
cilefen’s picture

cilefen CreditAttribution: cilefen commented

I did not realize it was test-only when I opened it. It is good to go if green.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry commented
FileSize
setdefault_on-2635784-11.patch9.39 KB

Backport to 8.0.x.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +london_2016_january

The patch is just a re-roll of original #2 version, just a small change on core/modules/ckeditor/src/Tests/CKEditorTest.php bit.

Setting the issue RTBC as I haven't really changed that match from the original patch (which has been already reviewed and tested).

ChandeepKhosa’s picture

ChandeepKhosa CreditAttribution: ChandeepKhosa at 2Toucans commented
Issue tags: +SprintWeekend2016
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Reviewed & tested by the community » Needs review

Thanks but this does still need peer review :)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Reviewed the patch, the only change is in the context, not the actual lines of the patch. I also re-grepped and didn't find any remaining occurrences after applying the patch. Thanks @gambry :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review

I'm not entirely sure what is the point of this change. Given that the default theme is a configuration setting it should be okay to just change that. If something needs to happen because the theme default theme has changed then it should be done in a config listener so if anything outside of the Drupal site changes it then it is respected. In fact once we have config validation I think we should consider removing ThemeHandler::setDefault() as being unnecessary and potentially dangerous as it encourages doing things in a way that could break config synchronisation. Basically I think this patch should be reverted from 8.1.x.

Setting to "needs review" so people can respond to my comment.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

That's worth discussing then, thanks @alexpott. What I'm not clear on from #16 is how wrapping the config write in setDefault() would be different/more problematic than calling it directly.

I just interpreted this change as using our APIs consistently but if our APIs are or may be problematic then we probably shouldn't add more of such uses, even if it's only in tests.

alexpott’s picture

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

Whilst setDefault only wraps config is it fine - but it encourages anything that overrides the theme handler to put additional logic there. Which would be wrong.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Could we make the method final?

(edited for typo)

alexpott’s picture

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

@cilefen but why? - my point is that drush config-set system.theme default needs to work because this is exactly the way that config synchronisation happens.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

@alexpott excellent point, I didn't quite get there. Then I agree with you, at least in tests it seems to make more sense to work with the config directly. I think discussing whether we want to deprecate setDefault() can be a separate issue.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.0.x-dev » 8.1.x-dev
FileSize
2635784-revert.patch9.41 KB

#16 is a good point, deprecating the method makes sense.

I went to revert in 8.1.x, but it didn't revert cleanly, so here's a patch.

Status: Needs review » Needs work

The last submitted patch, 22: 2635784-revert.patch, failed testing.

veerasekar89’s picture

veerasekar89 CreditAttribution: veerasekar89 at UniMity Solutions Pvt Limited commented
Assigned: Unassigned » veerasekar89
veerasekar89’s picture

veerasekar89 CreditAttribution: veerasekar89 at UniMity Solutions Pvt Limited commented
FileSize
setdefault_on-2635784.patch9.65 KB

I created a patch to fix \Drupal::service('theme_handler')->setDefault()

veerasekar89’s picture

veerasekar89 CreditAttribution: veerasekar89 at UniMity Solutions Pvt Limited commented
Status: Needs work » Needs review
arunkumark’s picture

arunkumark
Location Coimbatore
CreditAttribution: arunkumark as a volunteer and at UniMity Solutions Pvt Limited commented
Status: Needs review » Reviewed & tested by the community

Hi,
The patch seems good and working fine for all themes, thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: setDefault() on ThemeHandler is not used in all the places it could » Deprecate ThemeHandler::setDefault() in favour of configuration and replace usages
Status: Reviewed & tested by the community » Needs work

We should deprecate ThemeHandler::setDefault() for Drupal 9.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Category: Bug report » Task
Issue tags: -needs backport to 8.0.x +Needs issue summary update

This issue is not needed in 8.1.x there is no bug here. Also the issue summary needs to the updated to explain the new approach.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue summary: View changes
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue summary: View changes
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue tags: -Needs issue summary update

as much noise as possible...

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap at TATA Consultancy Services for Pfizer, Inc. commented

Is a new patch required against 8.2.x-dev here deprecating ThemeHandler::setDefault() ?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Yes, and replacing usages of it.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap at TATA Consultancy Services for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
setdefault-on-2635784-36.patch10.55 KB

Here is the initial patch against 8.2.x as per comment #35

Status: Needs review » Needs work

The last submitted patch, 36: setdefault-on-2635784-36.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented 
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -149,20 +149,6 @@ public function getDefault() {
   /**
    * {@inheritdoc}
    */
-  public function setDefault($name) {
-    $list = $this->listInfo();
-    if (!isset($list[$name])) {
-      throw new \InvalidArgumentException("$name theme is not installed.");

Deprecating is not the same as removing. It is a comment. And in this case, the interface must receive the comment.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap at TATA Consultancy Services for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
setdefault-on-2635784-39.patch10.29 KB

Updated patch.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
@@ -170,6 +170,9 @@ public function getDefault();
+   *
+   * @deprecated in Drupal 8.2.x-dev and will be removed before Drupal 9.0.0.
+   *   Use $this->config('system.theme')->set('default', $theme)->save();

This is better. But we cannot advise people to use an object method they may not have. It should be \Drupal::config().

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap at TATA Consultancy Services for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
setdefault-on-2635784-41.patch10.29 KB

Thank You @cilefen
Updated patch as per #40

  • catch committed 314de72 on 8.3.x 
    Issue #2635784 by cilefen: setDefault() on ThemeHandler is not used in...

  • catch committed 314de72 on 8.3.x 
    Issue #2635784 by cilefen: setDefault() on ThemeHandler is not used in...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

I agree, this is a bit of an abstraction for the sake of it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: setdefault-on-2635784-41.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Issue tags: +Needs reroll
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Issue tags: +Drupalaton
Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented
Assigned: veerasekar89 » Manuel Garcia

Rerolling...

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented
Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
deprecate-2635784-50.patch10.51 KB

The only conflict the rebase didn't handle was in core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php

Simple one to fix though so here it is =)

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented
FileSize
deprecate-2635784-51.patch10.5 KB

Erm.. removing extra line I added. Let's keep it clean =)

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Nice cleanup. There are a few others that are with 

$theme_handler->install(['seven']);
$theme_handler->setDefault('seven');

These are in tests mostly. Should we change these too? Otherwise good to commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Yep we shouldn't have any usages for the deprecation to be correct.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented
Status: Needs work » Needs review
FileSize
deprecate-2635784-55.patch15.46 KB
interdiff.txt4.96 KB

I think these were all that were left, please have a look.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

Thanks @Manuel Garcia. There are a few more.

core/modules/system/src/Tests/Theme/ThemeInfoTest.php:87
core/tests/Drupal/KernelTests/Core/Theme/StableThemeTest.php:50
core/tests/Drupal/KernelTests/Core/Theme/StableThemeTest.php:63
core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php:148

I just searched for ->setDefault( in the code base and excluded the route ones to find them all.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented
Status: Needs work » Needs review
FileSize
deprecate-2635784-57.patch17.94 KB
interdiff.txt2.47 KB

Ah good catch @joelpittet thanks!

Ran the same search I think these should be all now.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Good catches!

The only remaining setDefault calls are on routes.

catch’s picture

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

Committed/pushed to 8.3.x, thanks!

  • catch committed dd3ee4c on 8.3.x 
    Issue #2635784 by Manuel Garcia, sidharthap, cilefen, veerasekar89,...

Status: Fixed » Closed (fixed)

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