Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andreiashu’s picture

Status: Active » Needs review
FileSize
6.19 KB
andreiashu’s picture

Title: Move https variable to Configuration System » Convert https to CMI

updated title

Status: Needs review » Needs work

The last submitted patch, 1798832_https_1.patch, failed testing.

andreiashu’s picture

Status: Needs work » Needs review
FileSize
6.18 KB

moved default value to system.site.yml

Status: Needs review » Needs work

The last submitted patch, 1798832_https_2.patch, failed testing.

andreiashu’s picture

Status: Needs work » Needs review
FileSize
8.36 KB

I missed some a few tests

Status: Needs review » Needs work

The last submitted patch, 1798832_https_3.patch, failed testing.

andreiashu’s picture

Status: Needs work » Needs review

known random failure in EntityTranslationTest.php. Submitting for retest.

andreiashu’s picture

Issue tags: -Configuration system

#6: 1798832_https_3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1798832_https_3.patch, failed testing.

andreiashu’s picture

FileSize
8.36 KB

Attached patch also updates from variable to config

andreiashu’s picture

Status: Needs work » Needs review
andreiashu’s picture

FileSize
8.88 KB

Attached the patch that contains the actual hook_update_N

Status: Needs review » Needs work

The last submitted patch, 1798832_https_5.patch, failed testing.

andreiashu’s picture

FileSize
9.49 KB

The 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.

andreiashu’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1798832_https_6.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
9.44 KB

This is a reroll to HEAD, but I'm not sure what is causing those test failures.

Status: Needs review » Needs work

The last submitted patch, 1798832_https_18.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
9.3 KB

Maybe this fixes it...

aspilicious’s picture

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.phpundefined
@@ -474,11 +475,11 @@ function testLanguageDomain() {
+    $config->set('https', FALSE)->save();

Should we set with FALSE and TRUE or '0' and '1'? Will booleans be converted to 0 and 1?

znerol’s picture

diff --git a/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
index db95f06..2b720f9 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
@@ -73,6 +73,7 @@ public function testSystemVariableUpgrade() {
       'page.403' => '403',
       'page.404' => '404',
       'page.front' => 'node',
+      'https' => '0',
     );
 
     foreach ($expected_config as $file => $values) {

Where 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)

johan.gant’s picture

Issue tags: -Configuration system

#20: https_cmi.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, https_cmi.patch, failed testing.

johan.gant’s picture

From what I can tell, the patches on this ticket aim to set 'https' in system.install under update_8012.

johan.gant’s picture

Status: Needs work » Needs review
FileSize
9.34 KB

Attempted to re-roll the latest patch to work with latest HEAD version of D8. Hoping it'll pass...

Status: Needs review » Needs work

The last submitted patch, https_in_config_not_variables-1798832.patch, failed testing.

adammalone’s picture

Running locally:

  • Uncaught PHP Exception Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: "The service definition "config.factory" does not exist." at /home/d8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php line 690
  • Upgrade path test fails (even if not running the system upgrade path and instead running State system upgrade test [which has not been changed])
  • Language test passes
  • Session HTTPS test passes

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.

adammalone’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, https_in_config_not_variables-1798832-28.patch, failed testing.

ACF’s picture

Status: Needs work » Needs review
FileSize
8.46 KB

hopefully a fix.

Status: Needs review » Needs work

The last submitted patch, 1798832-https_to_config-drupal8-30.patch, failed testing.

xjm’s picture

Status: Needs work » Postponed

Thanks ACF!

The fatal in the test above is:

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "config.factory" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:740

This is probably #1848998: Problems with update_module_enable()? Let's postpone on fixing that.

Berdir’s picture

Status: Postponed » Needs review
Issue tags: -Configuration system

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1798832-https_to_config-drupal8-30.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.48 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, https-cmi-1798832-36.patch, failed testing.

Berdir’s picture

Hm, 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.

mfb’s picture

While 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.

catch’s picture

Title: Convert https to CMI » Convert https to $settings

I think this should be in $settings as well, there's no need for a UI and session handling can't depend on CMI.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.56 KB

Changed 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).

Status: Needs review » Needs work

The last submitted patch, https-1798832-41.patch, failed testing.

ParisLiakos’s picture

grep -r "variable_set('https" core/
core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:    variable_set('https', TRUE);
core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:    variable_set('https', FALSE);
core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.php:    variable_set('https', TRUE);
Berdir’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
8.65 KB

Wooo, it works!

ParisLiakos’s picture

awesome, 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

dawehner’s picture

It would be probably worth in url(), otherwise this seems RTBC for me.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

i 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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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