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 |
---|---|---|---|
#45 | https-1798832-44.patch | 8.65 KB | Berdir |
#45 | https-1798832-44-interdiff.txt | 2.09 KB | Berdir |
#41 | https-1798832-41.patch | 6.56 KB | Berdir |
#36 | https-cmi-1798832-36.patch | 8.48 KB | Berdir |
#31 | 1798832-https_to_config-drupal8-30.patch | 8.46 KB | ACF |
Comments
Comment #1
andreiashu CreditAttribution: andreiashu commentedComment #2
andreiashu CreditAttribution: andreiashu commentedupdated title
Comment #4
andreiashu CreditAttribution: andreiashu commentedmoved default value to system.site.yml
Comment #6
andreiashu CreditAttribution: andreiashu commentedI missed some a few tests
Comment #8
andreiashu CreditAttribution: andreiashu commentedknown random failure in EntityTranslationTest.php. Submitting for retest.
Comment #9
andreiashu CreditAttribution: andreiashu commented#6: 1798832_https_3.patch queued for re-testing.
Comment #11
andreiashu CreditAttribution: andreiashu commentedAttached patch also updates from variable to config
Comment #12
andreiashu CreditAttribution: andreiashu commentedComment #13
andreiashu CreditAttribution: andreiashu commentedAttached the patch that contains the actual hook_update_N
Comment #15
andreiashu CreditAttribution: andreiashu commentedThe tests failed in a different place for the patch at #13. I was useful though because I see we have SystemUpgradePathTest::testSystemVariableUpgrade() in which I added the test for the https variable upgrade.
Comment #16
andreiashu CreditAttribution: andreiashu commentedComment #18
gddThis is a reroll to HEAD, but I'm not sure what is causing those test failures.
Comment #20
aspilicious CreditAttribution: aspilicious commentedMaybe this fixes it...
Comment #21
aspilicious CreditAttribution: aspilicious commentedShould we set with FALSE and TRUE or '0' and '1'? Will booleans be converted to 0 and 1?
Comment #22
znerol CreditAttribution: znerol commentedWhere do you set the 'https' variable? I'd expect something in
core/modules/system/tests/upgrade/drupal-7.system.database.php
(see http://drupal.org/node/1667896)Comment #23
johan.gant CreditAttribution: johan.gant commented#20: https_cmi.patch queued for re-testing.
Comment #25
johan.gant CreditAttribution: johan.gant commentedFrom what I can tell, the patches on this ticket aim to set 'https' in system.install under update_8012.
Comment #26
johan.gant CreditAttribution: johan.gant commentedAttempted to re-roll the latest patch to work with latest HEAD version of D8. Hoping it'll pass...
Comment #28
adammaloneRunning locally:
Since system.site.yml has changed since the last patch with the addition of weight_select_max, here is a minor reroll so the patch applies cleanly.
Comment #29
adammaloneComment #31
ACF CreditAttribution: ACF commentedhopefully a fix.
Comment #33
xjmThanks ACF!
The fatal in the test above is:
This is probably #1848998: Problems with update_module_enable()? Let's postpone on fixing that.
Comment #34
Berdir#31: 1798832-https_to_config-drupal8-30.patch queued for re-testing.
Comment #36
BerdirRe-roll.
Comment #38
BerdirHm, wondering if we want to make this a setting instead?
Sounds like a possibly environment specific thing, that is usually already set in settings.php (see securepages project page), is called very early in update.php and similar places.
Comment #39
mfbWhile we're here, I don't think this setting should be called "https", since it is actually a harmful setting for those HTTPS sites that want to use only secure sessions. A more accurate description would be something like "mixed-mode sessions" because that's what it does: create both secure and insecure sessions when a session is created on an HTTPS site.
Comment #40
catchI think this should be in $settings as well, there's no need for a UI and session handling can't depend on CMI.
Comment #41
BerdirChanged to settings(), also renamed to mixed_mode_sessions and documented in default.settings.php
No upgrade path yet, we need the updated drupal_write_settings() for that. If we even want to. Still not sure if we want to migrate settings other than those that we will have have to (database, drupal hash salt).
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedComment #44
ParisLiakos CreditAttribution: ParisLiakos commentedthis needs #1949724: Allow simpletest child sites to additionally load a test-specific settings.php to allow testing anonymous and configless updates
Comment #45
BerdirWooo, it works!
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commentedawesome, thanks for rolling this!
i see settings() being called twice in some functions, we could store the object somewhere and reuse instead of keep calling settings()?
or you think its out of scope of this issue? its just that it got a bit uglier now
Comment #47
dawehnerIt would be probably worth in url(), otherwise this seems RTBC for me.
Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedi started cleaning it up, but then patch start getting bigger (especially in session.inc) , so lets get this in and cleanup elsewhere..variable_get() calls converted to settings()->get(), anything else is out of scope
Comment #49
catchCommitted/pushed to 8.x, thanks!