Replace $is_https global with Request::isSecure() [#1960344]

Comment	File	Size	Author
#16	drupal-replace_is_https-1960344-16.patch	11.63 KB	ParisLiakos

#12	drupal-replace_is_https-1960344-12.patch	11.67 KB	ParisLiakos

#12	interdiff.txt	634 bytes	ParisLiakos
#10	drupal-replace_is_https-1960344-10.patch	11.39 KB	ParisLiakos

#4	drupal-replace_is_https-1960344-4.patch	14.12 KB	kid_icarus

Comment #1

ParisLiakos CreditAttribution: ParisLiakos commented 3 April 2013 at 17:19

Status:

Active

» Postponed

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

Log in or register to post comments

Comment #2

ParisLiakos CreditAttribution: ParisLiakos commented 11 April 2013 at 00:22

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();

Log in or register to post comments

Comment #3

ParisLiakos CreditAttribution: ParisLiakos commented 11 April 2013 at 00:22

Status:

Postponed

» Active

blocker is in:)

Log in or register to post comments

Comment #4

kid_icarus CreditAttribution: kid_icarus commented 11 April 2013 at 01:34

Status:

Active

» Needs review

File	Size
drupal-replace_is_https-1960344-4.patch	14.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 :)

Log in or register to post comments

Comment #5

11 April 2013 at 05:58

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #6

Berdir

German

Switzerland

CreditAttribution: Berdir commented 11 April 2013 at 07:18

+++ 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

Log in or register to post comments

Comment #7

ParisLiakos CreditAttribution: ParisLiakos commented 11 April 2013 at 22:51

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);
}

Log in or register to post comments

Comment #8

kid_icarus CreditAttribution: kid_icarus commented 11 April 2013 at 15:29

@Berdir @rootawc

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

Log in or register to post comments

Comment #9

tsphethean CreditAttribution: tsphethean commented 12 April 2013 at 08:29

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

Log in or register to post comments

Comment #10

ParisLiakos CreditAttribution: ParisLiakos commented 14 April 2013 at 00:39

Status:

Needs work

» Needs review

File	Size
drupal-replace_is_https-1960344-10.patch	11.39 KB

happened to be in the hood..here we go

Log in or register to post comments

Comment #11

14 April 2013 at 01:34

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #12

ParisLiakos CreditAttribution: ParisLiakos commented 14 April 2013 at 02:03

Status:

Needs work

» Needs review

File	Size
interdiff.txt	634 bytes
drupal-replace_is_https-1960344-12.patch	11.67 KB

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

Log in or register to post comments

Comment #13

Crell CreditAttribution: Crell commented 14 April 2013 at 06:45

Status:

Needs review

» Reviewed & tested by the community

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

Log in or register to post comments

Comment #14

Damien Tournoud CreditAttribution: Damien Tournoud commented 14 April 2013 at 09:08

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.

Log in or register to post comments

Comment #15

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 15 April 2013 at 09:57

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

Log in or register to post comments

Comment #16

ParisLiakos CreditAttribution: ParisLiakos commented 26 April 2013 at 16:54

Status:

Postponed

» Needs review

File	Size
drupal-replace_is_https-1960344-16.patch	11.63 KB

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

Log in or register to post comments

Comment #17

ParisLiakos CreditAttribution: ParisLiakos commented 26 April 2013 at 19:35

Status:

Needs review

» Reviewed & tested by the community

back to rtbc

Log in or register to post comments

Comment #18

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 30 April 2013 at 15:50

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!

Log in or register to post comments

Comment #19

ParisLiakos CreditAttribution: ParisLiakos commented 30 April 2013 at 16:09

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