Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
ThemeHandler has a setDefault method which really is not appropriate. See #2416673-41: Add a ThemeConfig service for setting and getting default and admin themes.
Proposed resolution
- Add a ThemeConfig service that extends a (new) ThemeConfigInterface.
- Add methods: setDefault(), getDefault(), setAdmin() and getAdmin().
- Mark ThemeHandler::setDefault() as deprecated.
Remaining tasks
- Write a change record.
- Open a follow-up issue to replace usages of config->set(), config->get() for getting and setting default and admin themes.
- Open a follow-up issue to replace usages of ThemeHandler->setDefault() and ThemeHandler->getDefault()
User interface changes
API changes
Data model changes
Problem/Motivation
Comment | File | Size | Author |
---|---|---|---|
#68 | interdiff.txt | 1.43 KB | kgoel |
#68 | 2416673-68.patch | 12.5 KB | kgoel |
#66 | interdiff.txt | 2.72 KB | kgoel |
#66 | 2416673-66.patch | 12.48 KB | kgoel |
#64 | Interdiff.txt | 5.59 KB | kgoel |
Comments
Comment #1
cilefen CreditAttribution: cilefen commentedComment #2
humansky CreditAttribution: humansky commentedThis patch depends on #2409811: Kernel tests should explicitly install themes
Comment #3
cilefen CreditAttribution: cilefen commentedThis needs a reroll because #2409811: Kernel tests should explicitly install themes is in.
Comment #4
humansky CreditAttribution: humansky commentedre-rolling patch
Comment #5
humansky CreditAttribution: humansky commentedComment #6
tim.plunkettI just realized why this patch is so small.
A majority of these calls are split over multiple lines:
So a grep like this would find them:
grep -nrA1 "'system.theme'" * | grep "set('default'"
Comment #7
humansky CreditAttribution: humansky commentedThere are a few cases in which the admin theme is being set along side the default theme:
I want to propose we add a new API change in which we add two new API functions to the theme handler. See attached patch file. The patch will allow the above code to be rewritten as:
Comment #8
humansky CreditAttribution: humansky commentedComment #9
cilefen CreditAttribution: cilefen commented@Humansky It is either that or add a second parameter to setDefault? I prefer your suggestion.
Comment #10
tim.plunkettI much prefer a dedicated method. Setting the admin theme is a completely separate operation, despite often being done at the same time in tests.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedUsing PhpStorm / manual searching I found config('system.theme') saving in:
core/modules/block/src/Tests/BlockHiddenRegionTest.php line 59
core/modules/block/src/Tests/BlockTest.php line 207
core/modules/block/src/Tests/NewDefaultThemeBlocksTest.php line 49
core/modules/block_content/src/Tests/BlockContentTypeTest.php line 155
core/modules/ckeditor/src/Tests/CKEditorTest.php line 267
core/modules/color/src/Tests/ColorTest.php line 108 and 160
core/modules/comment/src/Tests/CommentLinksTest.php line 50
core/modules/migrate_drupal/src/Tests/d6/MigrateBlockTest.php line 63
core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php line 156
and so on.
But it looks like the fix is larger than that. We should probably fix all the getting as well.
Comment #12
humansky CreditAttribution: humansky commentedI agree with @cosmicdreams, we should also fix the getters as well. The attached patch fixes all the theme setters. I'll start working on the theme getters patch. Here is the grep command I'm using:
Which outputs these files:
Comment #15
tim.plunkettIt seems those patches were missing the actual changes to ThemeHandler itself
Comment #18
almaudoh CreditAttribution: almaudoh commentedCombined patches #7 and #12. Interdiff is patch #7
Comment #19
almaudoh CreditAttribution: almaudoh commentedOptimize code to avoid multiple calls to
\Drupal::service('theme_handler')
Comment #21
cosmicdreams CreditAttribution: cosmicdreams commented@almaudoh Oh cool, these can be chained? An excellent novice task.
Comment #22
tim.plunkettThey cannot be chained, install() doesn't return $this. @humansky and I discussed this in person, we should have mentioned it on issue.
Comment #23
almaudoh CreditAttribution: almaudoh commentedsetAdmin() and setDefault() can be chained. However #19 is about something like:
Now working on the three test fails which have to do with some really old code trying to set 'admin_theme' = '0' in ThemeTest and ConfigTranslationUITest.
Edit: it would be nice if install could be chained as well. Better DX.
Comment #24
almaudoh CreditAttribution: almaudoh commentedChanged the 'admin_theme' = '0' to a more explicit 'admin_theme' = '_default_' which
setAdmin()
recognizes as the default value. Don't know if this breaks anything...Comment #25
almaudoh CreditAttribution: almaudoh commentedComment #26
tim.plunkettThis is completely out of scope, AFAICS.
Hmm, this change worries me. Might have to ping @alexpott directly for his opinion.
Comment #27
almaudoh CreditAttribution: almaudoh commented@tim.plunkett: 1) the change is needed to allow the tests pass. We may have to raise another issue to look into that pre-existing bug. 2) This change is not really needed for the patch but that piece of code would be redundant.
I don't think this issue is as straightforward as originally thought.
Comment #28
markhalliwellThis should account for when the admin theme set to
0
, which is supposed to use the default theme. See related issue for an example of what occurs when this isn't checked.Comment #30
almaudoh CreditAttribution: almaudoh commentedRe-rolled
Comment #31
almaudoh CreditAttribution: almaudoh commentedComment #32
dawehnerThat is an API change now, so this is certainly at least 8.1.x work.
Comment #33
markhalliwellSetting back to CNW per #28.
@dawehner, I'm not sure how this is technically a "change". If anything, it's adding in the missing pieces of ThemeManager that should have been added before 8.0.0 was released. It doesn't prevent anyone from still using config.
Comment #34
tim.plunkettThese constitute a BC break.
Comment #35
markhalliwellThe definition of "BC break" being used in this issue is very far reaching.
You cannot break something that didn't exist before (the methods, not the class/interface). If this patch were changing existing signature methods,then sure... that would constitute a BC break... but it's not, it's just a simple addition.
Comment #36
cilefen CreditAttribution: cilefen commented@markcarver New features, including API extensions, must go to 8.1.x.
Comment #37
markhalliwellWhich is why I didn't change the version... yet.
However, I seriously don't understand how fixing a prior feature (and incomplete I might add) (#2228093: Modernize theme initialization) also makes this task a "feature". The theme handler/manager/service stuff is great and all, but it wasn't fully baked and left a lot of stuff slide. The fact that we have errors like #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense is just an example of this. If anything, this is more of a follow-up to add in the missing pieces that were simply overlooked.
So, if the powers that be want to redefine what a "BC break" is and call this a "feature", then I guess we can leave the version as 8.1.x... otherwise adding in something so simple to help fix issues like the above seems like a better win in my book.
Comment #38
cilefen CreditAttribution: cilefen commentedLet's ask the release manager for an opinion.
Comment #39
tim.plunkett@markcarver
If in contrib I write a class called MyBetterThemeHandler:
And then this patch gets committed, my class will be broken and cause fatal errors.
Adding methods to the interface is a BC break.
Comment #40
markhalliwellOk, I see the BC break now. I seriously doubt though that single soul has implemented this interface other than core...
Regardless, would an acceptable compromise be to comment out the interface additions with a @todo that they should be added back in 8.1.x or just create a 8.1.x follow up issue for the interface additions?
This way we can add the missing and needed methods?
Comment #41
dawehnerMoving to the theme system where it belongs to.
A bit more of a provocative question: Should those methods really be on the ThemeHandler? It feels like setDefault() was placed there in order to have a dumping ground for methods related to themes. A simple
ThemeConfig
or so could have setDefault/and all the various other ones, which then would no longer be a direct BC break.Comment #42
almaudoh CreditAttribution: almaudoh commented@dawehner, I agree
ThemeHandler
is not the right place for these methods.Comment #43
xjmThanks @cilefen for tagging!
I agree that #41 sounds like a safer (and more architecturally sound) approach.
Either solution should go into the next/development minor branch and is not eligible for a patch release, since it includes API additions and (in the case of the current patch) BC breaks for an interface and base class. References: https://www.drupal.org/core/d8-allowed-changes#minor and https://www.drupal.org/core/d8-bc-policy
Comment #44
cilefen CreditAttribution: cilefen commentedOk, this has now become two issues. I am reframing this one to be the API extension because it contains the discussion on that and I opened #2635784: Deprecate ThemeHandler::setDefault() in favour of configuration and replace usages for the original reason this issue was opened, which is simple.
Comment #45
cilefen CreditAttribution: cilefen commentedComment #46
cilefen CreditAttribution: cilefen commentedThis is a work-in-progress on the new plan. @dawehner - is this on the right track?
Comment #47
cilefen CreditAttribution: cilefen commentedI updated the issue summary to indicate we should open follow-up issues to fix usages. This patch needs tests.
Comment #48
cilefen CreditAttribution: cilefen commentedComment #49
almaudoh CreditAttribution: almaudoh commentedThe implementations in
ThemeHandler
should be changed to delegate to theThemeConfig
serviceI suggest we should name these
(get|set)DefaultTheme
and(get|set)AdminTheme
so it is self documenting.Comment #50
cilefen CreditAttribution: cilefen commentedRegarding #49:
That is an API change. Is that allowed?
Comment #51
almaudoh CreditAttribution: almaudoh commentedIt shouldn't be an API change.
Done #49 and also renamed the class to
ThemeConfiguration
andThemeConfigurationInterface
. If that's too verbose, then feel free to revert.Comment #52
dawehnerMh, so ThemeHandler should be certainly changed, IMHO.
Here is a general question, when we would do that (call to ThemeConfig from ThemeHandler), we would have a circular dependency. I think its fine to have circular dependencies in BC layers ...
Comment #53
cilefen CreditAttribution: cilefen commentedShall we close this based on #2416673-28: Add a ThemeConfig service for setting and getting default and admin themes?
Comment #54
dawehner@cilefen
Is this really the issue you wanted to link to?
Comment #55
cilefen CreditAttribution: cilefen commentedSorry, #2635784-28: Deprecate ThemeHandler::setDefault() in favour of configuration and replace usages
Comment #56
dawehner@cilefen
Well no, actually this issue is about doing it right on the architecture side of things. #2635784: Deprecate ThemeHandler::setDefault() in favour of configuration and replace usages just ensures that we use the old api consistent.
Comment #57
dawehnerBeside of the tests and the change record this looks really good!
Comment #59
kgoel CreditAttribution: kgoel as a volunteer commentedComment #60
cilefen CreditAttribution: cilefen commented@kgoel It looks like some migrate stuff got in the #59 patch.
Comment #61
kgoel CreditAttribution: kgoel at Forum One commentedIt sure did. trying again
Comment #62
tim.plunkettIMO this should document that it CANNOT be injected due to a circular dependency, and maybe refer to the fact that it should go away once getDefault/setDefault are removed.
Missing blank line
This test is already using prophecy, might as well use it consistently.
Also can use the ::class constant
Missing blank line
Comment #63
cilefen CreditAttribution: cilefen commentedThe draft change record, please edit as you will.
Comment #64
kgoel CreditAttribution: kgoel commentedComment #66
kgoel CreditAttribution: kgoel commentedComment #67
dawehnerAt that time we cannot target 8.1 anymore, so this needs to be moved to 8.2
This documentation is IMHO a bit weak, maybe a bit more description of what is going on. This interface here is one example of something I just don't get why we need an interface. Why would you swap out that behaviour ever.
Comment #68
kgoel CreditAttribution: kgoel commentedAddressed both 67.1 and 67.2
Comment #69
dawehnerCool, thank you!
Comment #70
alexpottSo afaics ThemeHandler::setDefault() is only used in tests and ::getDefault() only has 2 usages - one outside tests. So there must be plenty of places where we are just reading and writing to config directly. I think we should just deprecate the methods and tell people to use config. Because adding a ThemeConfig service implies we could swap out the service and make it store the default theme somewhere other than config - that's not going to work.
If I'm wrong then we need a better justification of adding the new service in the issue summary.
Comment #71
alexpottSetting back to needs work for #70 since at the very least the issue summary needs an update.
Comment #72
star-szrWhat @alexpott said makes sense to me, I would love to see that justification.