Lets get rid of this global..Symfony's request provides isSecure method which is better

CommentFileSizeAuthor
#16 drupal-replace_is_https-1960344-16.patch11.63 KBParisLiakos
#12 drupal-replace_is_https-1960344-12.patch11.67 KBParisLiakos
#12 interdiff.txt634 bytesParisLiakos
#10 drupal-replace_is_https-1960344-10.patch11.39 KBParisLiakos
#4 drupal-replace_is_https-1960344-4.patch14.12 KBkid_icarus

Comments

ParisLiakos’s picture

ParisLiakos commented
Status: Active » Postponed

well, i ll wait for #1847768: Remove ip_address() i kinda have to do the same work i did there already

ParisLiakos’s picture

ParisLiakos commented
Assigned: ParisLiakos » Unassigned
Issue tags: +Novice

wont be that hard after the issue above is in, so unnassigning and tagging novice
Replace $is_https global variable with Drupal::request()->isSecure();

ParisLiakos’s picture

ParisLiakos commented
Status: Postponed » Active

blocker is in:)

kid_icarus’s picture

kid_icarus commented
Status: Active » Needs review
StatusFileSize
new drupal-replace_is_https-1960344-4.patch14.12 KB

Ok, here is my first attempt. I think this is pretty close to there, except for one little piece.

I'm confused on how to treat this little snippet at the bottom of core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php (lines 470-477).

I'm not quite sure what this is doing, but what raises a red flag for me is assigning $is_https to TRUE:

    // Test HTTPS via current URL scheme.
    $temp_https = $is_https;
    $is_https = TRUE;
    $italian_url = url('admin', array('language' => $languages['it'], 'script' => ''));
    $correct_link = 'https://' . $link;
    $this->assertTrue($italian_url == $correct_link, format_string('The url() function returns the right URL (via current URL scheme) (@url) in accordance with the chosen language', array('@url' => $italian_url)));
    $is_https = $temp_https;
  }

Feedback greatly appreciated :)

Status: Needs review » Needs work

The last submitted patch, drupal-replace_is_https-1960344-4.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionHttpsTest.phpundefined
@@ -30,9 +30,8 @@ public static function getInfo() {
-    if ($is_https) {
+    if (Drupal::request()->isSecure()) {

If you use this in classes, you need \Drupal

ParisLiakos’s picture

ParisLiakos commented

thank you kid_icarus!
in drupal_settings_initialize() is probably too early, we dont even have a kernel yet, so i guess we can leave the old logic there but just remove the global declaration on the top

about the test, you should create a dummy request in the test setUp() function, store it to $this->request and set it to the container..then when it is assigning the global variable to TRUE you could set the server https variable to "on" or 1

Something like this:

use Symfony\Component\HttpFoundation\Request;

function setUp() {
  $this->request = Request::create('http://example.com/');
  $this->container->set('request', $this->request);
}

function testHttps() {
  // Now isSecure() returns TRUE
  $this->request->server->set('HTTPS', 1);
}
kid_icarus’s picture

kid_icarus commented

@Berdir @rootawc

Thank you for the feedback :) I'll address this tonight!

tsphethean’s picture

tsphethean commented

This is also a sub-task of #1956180: [meta] Remove global variables

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-replace_is_https-1960344-10.patch11.39 KB

happened to be in the hood..here we go

Status: Needs review » Needs work

The last submitted patch, drupal-replace_is_https-1960344-10.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt634 bytes
new drupal-replace_is_https-1960344-12.patch11.67 KB

what a nice hacky way we have to test https:)

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

The conversions in #12 all look reasonable. Thanks, team!

damien tournoud’s picture

damien tournoud commented

In drupal_settings_initialize() is probably too early, we dont even have a kernel yet, so i guess we can leave the old logic there but just remove the global declaration on the top

Yes, that's part of the bootstrap cleanup that need to happen. Our whole container (and as a consequence the whole kernel) depends on the request (because the configuration path depends on the request). So we need to change the bootstrap from:

$kernel = new DrupalKernel();
$kernel->boot();
$request = Request::createFromGlobals();
$response = $kernel->handle($request)->prepare($request)->send();

... into something more like:

$request = Request::createFromGlobals();
$kernel = DrupalKernel::createFromRequest($request);
$kernel->boot();
$response = $kernel->handle($request)->prepare($request)->send();

And at the same time, we can implement DrupalKernel::createForSite($url) for command line tools like Drush to use.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Postponed

Postponing on #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence as this patch conflicts

ParisLiakos’s picture

ParisLiakos commented
Status: Postponed » Needs review
StatusFileSize
new drupal-replace_is_https-1960344-16.patch11.63 KB

issue above has no longer avoid commit conflicts tag, here is a reroll since previous patch stopped applying

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

back to rtbc

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Replace $is_https global with Request::isSecure() » Change notification: Replace $is_https global with Request::isSecure()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

I think #14 is part of a larger issue and this patch gets us some of the way there and removes a global to boot :)

I've tested installation under https and general usage everything appears to be working just fine.

Committed 2784f48 and pushed to 8.x. Thanks!

ParisLiakos’s picture

ParisLiakos commented
Title: Change notification: Replace $is_https global with Request::isSecure() » Replace $is_https global with Request::isSecure()
Status: Active » Fixed
Issue tags: -Needs change record

yay!
change notice: https://drupal.org/node/1983438
related meta #1956180: [meta] Remove global variables

ParisLiakos’s picture

ParisLiakos commented
Priority: Critical » Normal

status

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