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 |
---|---|---|---|
#37 | maintenance-1829170-37.patch | 3.91 KB | ianthomas_uk |
#37 | 1829170-interdiff-31-37.txt | 2.17 KB | ianthomas_uk |
#31 | interdiff.txt | 1.49 KB | dawehner |
#31 | maintaince-1829170-31.patch | 3.46 KB | dawehner |
#29 | drupal8.theme-system.1829170-29.patch | 3.09 KB | andypost |
Comments
Comment #1
pfrenssenThis is blocked by #1829224: Convert the 'theme_default' variable to CMI.
Comment #2
sunComment #3
catchNo longer blocked.
This has to work when the database is down so it should be in settings rather than CMI.
Comment #4
vijaycs85Updating as $conf as we got
$conf['system.theme']['default']
Comment #5
gddcatch do you have a problem with leaving this in $conf? I'd kind of prefer it myself.
Nitpick: this comment is over 80 characters
Comment #6
socketwench CreditAttribution: socketwench commentedReorganized comment to fit 80 character limit.
Comment #7
socketwench CreditAttribution: socketwench commentedReorganized comment to fit 80 character limit.
Comment #8
vijaycs85#7: 1829170-maintenance-theme-cmi-6.patch queued for re-testing.
Comment #10
vijaycs85Re-rolling...
Comment #11
ACF CreditAttribution: ACF commented#10: 1829170-maintenance-theme-cmi-10.patch queued for re-testing.
Comment #12
LittleCodingRemoving trailing whitespace.
Comment #13
vijaycs85#12: 1829170-maintenance-theme-cmi-12.patch queued for re-testing.
Comment #14
LinL CreditAttribution: LinL commentedUpdating for new Twig template location.
Comment #15
LinL CreditAttribution: LinL commentedUpdated to Drupal::config() as per #2028149: The config() function should be deprecated in favour of Drupal::config()
Also took out my change in #14 as that is probably scope creep.
Comment #16
vijaycs85#15: 1829170-maintenance-theme-cmi-15.patch queued for re-testing.
Comment #17
star-szrThe change in #14 is fair game since we are already changing that line. If we are changing the line we should fix it!
Comment #18
LinL CreditAttribution: LinL commentedThanks Cottser, never sure about ones like that :)
Comment #20
LinL CreditAttribution: LinL commented#18: 1829170-maintenance-theme-cmi-18.patch queued for re-testing.
Comment #22
star-szr#18: 1829170-maintenance-theme-cmi-18.patch queued for re-testing.
Comment #23
aspilicious CreditAttribution: aspilicious commentedI can be wrong but this will return a boolean in stead of the maintenance theme. The ternary shorthand with isset is dangerous :)
Comment #24
mtift@aspilicious you are correct. An updated patch is attached that also includes a minor change to the text.
Comment #25
dawehnerWith the patch applied the logic is really long:
This seems to be really duplicated information
Comment #26
mtift@dawehner: thanks for the review. I agree. Updated patch is attached.
Comment #27
dawehnerThis is indeed way easier :)
Comment #28
alexpottSo I don't think we should be using CMI here at all we should be using the Settings object since this should only be configured in settings.php as it is used when the database is unavailable.
Comment #29
andypostCMI could use a kind of caching and needs directories configured so let's use $settings. (#3 catch and #28 alexpott suggest that)
There's a bit of confusion because the same settings used for install-update but defaults to 'seven', probably this should be preserved for custom profile installers
Comment #31
dawehnerYeah the required logic is kind of odd.
Comment #32
andypostMakes sense!
Comment #33
aspilicious CreditAttribution: aspilicious commentedBack to rtbc. looks ok to me...
Comment #34
vijaycs85+1 for RTBC.
Comment #35
catchRather than checking for $config being true and an object, should we be doing try/catch there instead?
Comment #36
webchickSounds like a "needs review"
Comment #37
ianthomas_ukHere is a patch using try/catch.
I've also reworked the comments and split the assignment inside the if into two separate lines, to help readability.
Comment #38
catchLooks much better to me.
Comment #39
catchCommitted/pushed to 8.x, thanks!