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.
Comment | File | Size | Author |
---|---|---|---|
#36 | 1829224.convert_theme_default.36.patch | 1.4 KB | ianthomas_uk |
#30 | 28-30-interdiff.txt | 1.71 KB | alexpott |
#30 | 1829224.convert_theme_default.30.patch | 42.51 KB | alexpott |
#28 | 26-28-interdiff.txt | 2.48 KB | alexpott |
#28 | 1829224.convert_theme_default.28.patch | 42.5 KB | alexpott |
Comments
Comment #1
pfrenssenHere's a first patch.
I want to take a look at
_drupal_maintenance_theme()
again after the test.Comment #2
pfrenssenComment #4
leschekfm CreditAttribution: leschekfm commentedThe changes seem to work. Changing the theme settings reflects in changes to the config files and the db.
I'm not sure why the simpletests fail. When the simpletest are run, the test tries to delete the temporarily set config items from simpletestcache_config which is missing.
Comment #5
pfrenssenThanks for the review! I'm looking into it with help from fubhy and alexpott. It seems to have to do with the naming of the config object.
Comment #6
pfrenssenI changed the config name from system.theme.default to system.site.theme_default on alexpotts request.
Comment #7
pfrenssenI'm going to git bisect this to figure out where it goes wrong. This command puts each changed file in a separate commit to facilitate this. Just a reminder for myself.
Comment #8
pfrenssenThis is the change that causes the tests to fail:
Comment #9
alexpottFor this patch to work we need #1770902: Theme of parent site executing test leaks into all tests to land...
The change to drupal_theme_initialize() means that the full container is being instantiated in DrupalUnitTestBase :( due to the called to drupal_theme_initialize in TestBase::prepareEnvironment().
Comment #10
alexpottTagging and setting status.
Comment #11
mbrett5062 CreditAttribution: mbrett5062 commented@alexpott I have just got #1770902: Theme of parent site executing test leaks into all tests, green, a minor change to @sun's code. If you could review that and mark RTBC, then we can get this unblocked, and I will get #1067408: Themes do not have an installation status up to scratch with this change. I would like to see the tangled theme issues resolved. Thanks in advance for your help.
Comment #12
pfrenssenUnassigning myself.
Comment #13
lirantal CreditAttribution: lirantal commentedI actually filed #1876534: Convert theme_default setting to configuration system today, after doing a bit of searching and yet still missed this bug so I guess that I can mark it a duplicate of this bug.
See updated patch please.
Comment #14
lirantal CreditAttribution: lirantal commentedI think this still might fail as we need to also accommodate for other variables like maintenance_theme etc but let's wait and see.
Comment #16
vijaycs85Changes in this patch:
1. Updated patch to work with current code base.
2. Updated maintenance_theme
Comment #18
vijaycs85#16: 1829224-convert_theme_default_var-reroll-16.patch queued for re-testing.
Comment #19
lirantal CreditAttribution: lirantal commentedWill hopefully take another look at this soon and provide an up to date patch.
Comment #20
vijaycs85Comment #21
vijaycs85Re-rolling...
Comment #23
vijaycs85#21: 1829224-convert_theme_default_var-21.patch queued for re-testing.
Comment #25
alexpottThere are loads more usages of the theme_default variable with the blocks to plugins conversions.
Fingers-crossed this patch might be green...
One thing that was interesting is that we need to the default theme config in DrupalUnitTestBase so that any of the many unit tests that use theme functions work. In some ways you could argue that this tests should actually declared this dependency and enable stark and set it to the default theme... maybe @sun can chime in with an opinion?
No interdiff as the patch has doubled in size so reviewers should start again...
Comment #26
alexpottForgot sites/default/default.settings.php
Comment #27
EclipseGc CreditAttribution: EclipseGc commentedThis looks very sane to my eyes.
Comment #28
alexpottIn talking with @heyrocker on irc realised that maintenance_theme variable changes have snuck into this patch... the patch attached sneaks them back out again in favour of #1829170: Convert the Maintenance Theme variable to settings
Comment #30
alexpottOops... need to ensure bartik default in maintenance theme...
Comment #31
gddI've looked this over and it looks good to me too. So happy to get this in! W00t!
Comment #32
catchCommitted/pushed to 8.x, thanks!
Comment #33
sunLooks great.
re: #25:
I wasn't aware of this separate issue and was only following the existing theme config conversion issues. Feel free to ping me privately or simply tag issues with 'sun'.
Hm. So what you're essentially doing is to
1) Install the single system.theme config file.
2) Enable the Stark theme.
3) Configure Stark as default theme.
Apparently, step 2) is missing. Step 1) doesn't happen, and we aren't able to install single config files yet. And step 3) only works, because the current theme extension system is lame.
Also noteworthy:
system_list()
's static cache is not guaranteed to be reset. DrupalKernel isn't updated for the new theme.This will cause us quite some additional fun in #1067408: Themes do not have an installation status...
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedReopening because in HEAD, BlockTest and MenuTest still have one line each of
variable_get('theme_default', 'stark')
.Comment #36
ianthomas_ukHere's the fix for the tests.
Comment #37
catchComment #39
ianthomas_uk#36: 1829224.convert_theme_default.36.patch queued for re-testing.
Comment #40
ianthomas_ukRe-applying RTBC by catch in #37
Comment #41
alexpottCommitted 6ec7f3d and pushed to 8.x. Thanks!