Use Symfony Request for language module [#1999388]

Comment	File	Size	Author
#34	1999388-language-request.interdiff.29-34.txt	216 bytes	penyaskito
#34	1999388-language-request-34.patch	6.08 KB	penyaskito

#35	1999388-language-request.interdiff.34-35.txt	1.03 KB	penyaskito
#35	1999388-language-request-35.patch	6.34 KB	penyaskito

#29	1999388-language-request-29.patch	6.06 KB	herom

#29	interdiff-1999388-24-29.txt	5.81 KB	herom
#26	1999388-language-request-24.patch	1.7 KB	wouter.adem

#25	1999388-language-request-24.patch	1.7 KB	wouter.adem

#24	1999388-language-request-24.patch	1.7 KB	wouter.adem

#18	1999388-language-request-18.patch	4.99 KB	kim.pepper

#18	interdiff.txt	1.68 KB	kim.pepper
#13	1999388-language-request-13.patch	4.98 KB	kim.pepper

#11	1999388-language-request-11.patch	4.98 KB	kim.pepper

#9	1999388-9_replace-raw-variables-language.patch	10.02 KB	pwieck

#5	1999388-5_replace-raw-variables-language.patch	10.06 KB	pwieck

#3	1999388-3_replace-raw-variables-language.patch	7.25 KB	chertzog

Comment #1

tyjamessmith CreditAttribution: tyjamessmith commented 24 May 2013 at 19:02

Assigned:

Unassigned

» tyjamessmith

tyjamessmith is working on this one

Log in or register to post comments

Comment #2

tyjamessmith CreditAttribution: tyjamessmith commented 24 May 2013 at 19:21

Assigned:

tyjamessmith

» Unassigned

Having some problems, feel free to take over on this one.

Log in or register to post comments

Comment #3

chertzog

he/him

English

Harrisburg, PA, USA

CreditAttribution: chertzog commented 28 May 2013 at 13:08

Status:

Active

» Needs review

File	Size
1999388-3_replace-raw-variables-language.patch	7.25 KB

Lets give this a try.

Log in or register to post comments

Comment #4

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 1 June 2013 at 21:09

Status:

Needs review

» Needs work

+++ b/core/modules/language/language.negotiation.incundefined
@@ -86,7 +86,8 @@ function language_from_interface() {
+  $request = Drupal::request()->server->get('HTTP_ACCEPT_LANGUAGE');

Request doesn't seem the right variable name to use. How about $acceptLang ?

+++ b/core/modules/language/language.negotiation.incundefined
@@ -244,7 +245,8 @@ function language_from_session($languages) {
+  $request = Drupal::request()->query->get($param);

Again. How about $queryLang?

+++ b/core/modules/language/language.negotiation.incundefined
@@ -564,7 +566,8 @@ function language_url_rewrite_session(&$path, &$options) {
+      $query = Drupal::request()->query->get($query_param);

$query isn't the right variable name to use. How about $sessionParam?

Log in or register to post comments

Comment #5

pwieck CreditAttribution: pwieck commented 17 June 2013 at 07:27

Status:

Needs work

» Needs review

File	Size
1999388-5_replace-raw-variables-language.patch	10.06 KB

Made changes as suggested by kim.pepper in comment #4 plus re-roll

Log in or register to post comments

Comment #6

17 June 2013 at 08:38

Status:

Needs review

» Needs work

The last submitted patch, 1999388-5_replace-raw-variables-language.patch, failed testing.

Log in or register to post comments

Comment #7

pwieck CreditAttribution: pwieck commented 17 June 2013 at 13:01

Status:

Needs work

» Needs review

#5: 1999388-5_replace-raw-variables-language.patch queued for re-testing.

Log in or register to post comments

Comment #8

17 June 2013 at 14:23

Status:

Needs review

» Needs work

The last submitted patch, 1999388-5_replace-raw-variables-language.patch, failed testing.

Log in or register to post comments

Comment #9

pwieck CreditAttribution: pwieck commented 17 June 2013 at 15:42

Status:

Needs work

» Needs review

File	Size
1999388-9_replace-raw-variables-language.patch	10.02 KB

Since I seem to have done something wrong this is just a standard re-roll to work against without changes from #4

Log in or register to post comments

Comment #10

17 June 2013 at 16:48

Status:

Needs review

» Needs work

The last submitted patch, 1999388-9_replace-raw-variables-language.patch, failed testing.

Log in or register to post comments

Comment #11

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 16 July 2013 at 12:15

Status:

Needs work

» Needs review

File	Size
1999388-language-request-11.patch	4.98 KB

Looks like there was code from other issues in that last commit. This one is clean start from the current head.

Log in or register to post comments

Comment #12

16 July 2013 at 13:33

Status:

Needs review

» Needs work

The last submitted patch, 1999388-language-request-11.patch, failed testing.

Log in or register to post comments

Comment #13

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 23 August 2013 at 22:57

Status:

Needs work

» Needs review

File	Size
1999388-language-request-13.patch	4.98 KB

Re-roll.

Log in or register to post comments

Comment #14

marcingy CreditAttribution: marcingy commented 23 August 2013 at 23:00

Status:

Needs review

» Reviewed & tested by the community

Looks good assuming comes back green

Log in or register to post comments

Comment #15

24 August 2013 at 00:22

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 1999388-language-request-13.patch, failed testing.

Log in or register to post comments

Comment #16

star-szr

he/him

English

CreditAttribution: star-szr commented 18 September 2013 at 02:09

Issue tags:

+Needs reroll

Looks like this needs a reroll.

Log in or register to post comments

Comment #17

andypost

he/him

Russian

CreditAttribution: andypost commented 21 September 2013 at 01:45

Log in or register to post comments

Comment #18

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 22 September 2013 at 04:41

Status:	Needs work	» Needs review
Issue tags:	-Needs reroll

File	Size
interdiff.txt	1.68 KB
1999388-language-request-18.patch	4.99 KB

Re-roll and use backslash for \Drupal as per #2053489: Standardize on \Drupal throughout core

Log in or register to post comments

Comment #19

22 September 2013 at 06:13

Status:	Needs review	» Needs work
Issue tags:		+Needs reroll

The last submitted patch, 1999388-language-request-18.patch, failed testing.

Log in or register to post comments

Comment #20

andypost

he/him

Russian

CreditAttribution: andypost commented 23 September 2013 at 17:02

+++ b/core/modules/language/language.negotiation.inc
@@ -253,7 +254,8 @@ function language_from_session($languages) {
-  if (isset($_GET[$param]) && isset($languages[$langcode = $_GET[$param]])) {
...
+  if (!empty($langcode) && isset($languages[$langcode])) {

No assign should happen when no $param in $_GET
$query = \Drupal::request()->query;
if ($query->has($param) && isset($languages[$langcode = $query->get($param)]))

+++ b/core/modules/language/language.negotiation.inc
@@ -496,7 +498,7 @@ function language_url_rewrite_session(&$path, &$options) {
-      $query_value = isset($_GET[$query_param]) ? check_plain($_GET[$query_param]) : NULL;
+      $query_value = check_plain(\Drupal::request()->query->get($query_param));

query->has() ? check_plain() : NULL

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php
@@ -154,7 +155,9 @@ function testLanguageFromBrowser() {
-      $_SERVER['HTTP_ACCEPT_LANGUAGE'] = $accept_language;
...
+      $request->server->set('HTTP_ACCEPT_LANGUAGE', $accept_language);
+      \Drupal::getContainer()->set('request', $request);

+++ b/core/modules/language/tests/language_test/lib/Drupal/language_test/LanguageTestManager.php
@@ -20,7 +20,9 @@ class LanguageTestManager extends LanguageManager {
-      $_SERVER['HTTP_HOST'] = $test_domain;
...
+      $request->server->set('HTTP_HOST', $test_domain);
+      \Drupal::getContainer()->set('request', $request);

This could not work!

Log in or register to post comments

Comment #21

no_angel CreditAttribution: no_angel commented 26 September 2013 at 10:52

Issue tags:

-Needs reroll

removed "needs reroll" tag.

Log in or register to post comments

Comment #22

no_angel CreditAttribution: no_angel commented 26 September 2013 at 10:53

removed "needs reroll" tag.

Log in or register to post comments

Comment #23

no_angel CreditAttribution: no_angel commented 26 September 2013 at 10:55

Issue tags:

+Needs reroll

removed "needs reroll" tag.

Log in or register to post comments

Comment #24

wouter.adem CreditAttribution: wouter.adem commented 27 September 2013 at 11:09

File	Size
1999388-language-request-24.patch	1.7 KB

Log in or register to post comments

Comment #25

wouter.adem CreditAttribution: wouter.adem commented 27 September 2013 at 11:10

Issue tags:

-Needs reroll

File	Size
1999388-language-request-24.patch	1.7 KB

Created patch

Log in or register to post comments

Comment #26

wouter.adem CreditAttribution: wouter.adem commented 27 September 2013 at 11:16

Status:	Needs work	» Needs review
Issue tags:		+Needs reroll

File	Size
1999388-language-request-24.patch	1.7 KB

Fix language request

Log in or register to post comments

Comment #27

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 29 September 2013 at 22:16

Thanks for this, however you are missing quite a few conversions:

language/language.negotiation.inc:84:  if (empty($_SERVER['HTTP_ACCEPT_LANGUAGE'])) {
language/language.negotiation.inc:96:  if (preg_match_all('@(?<=[, ]|^)([a-zA-Z-]+|\*)(?:;q=([0-9.]+))?(?:$|\s*,\s*)@', trim($_SERVER['HTTP_ACCEPT_LANGUAGE']
language/language.negotiation.inc:305:      $http_host= $_SERVER['HTTP_HOST'];
language/language.negotiation.inc:415:  $query = $_GET;
language/lib/Drupal/language/Tests/LanguageBrowserDetectionUnitTest.php:157:      $_SERVER['HTTP_ACCEPT_LANGUAGE'] = $accept_language;
language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:345:      // Language domain specific URL, we set the $_SERVER['HTTP_HOST'] in
language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:479:    $request = Request::create('', 'GET', array(), array(), array(), array('HTTP
language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php:128:    $request = $this->prepareRequestForGenerator(TRUE, array('SERVER_PORT' => '88'));
language/tests/language_test/lib/Drupal/language_test/Controller/LanguageTestController.php:107:    $sub_request = Request::create($base_path . '/user', 'GET
language/tests/language_test/lib/Drupal/language_test/LanguageTestManager.php:23:      $_SERVER['HTTP_HOST'] = $test_domain;

Also there are a few issues with trailing whitespace in your patch. See https://drupal.org/coding-standards#indenting for more information.

+++ b/core/modules/language/language.negotiation.inc
@@ -250,21 +250,22 @@ function language_from_user_admin(array $languages, Request $request = NULL) {
+  ¶

White space

+++ b/core/modules/language/language.negotiation.inc
@@ -250,21 +250,22 @@ function language_from_user_admin(array $languages, Request $request = NULL) {
+  ¶

Whitespace

+++ b/core/modules/language/language.negotiation.inc
@@ -250,21 +250,22 @@ function language_from_user_admin(array $languages, Request $request = NULL) {
+  } ¶

Whitespace

+++ b/core/modules/language/language.negotiation.inc
@@ -496,7 +497,13 @@ function language_url_rewrite_session(&$path, &$options) {
+        return FALSE; ¶

Whitespace

Log in or register to post comments

Comment #28

kim.pepper

English

🏄‍♂️🇦🇺Sydney, Australia

CreditAttribution: kim.pepper commented 29 September 2013 at 22:15

Status:

Needs review

» Needs work

double post

Log in or register to post comments

Comment #29

herom CreditAttribution: herom commented 7 October 2013 at 21:01

Status:

Needs work

» Needs review

File	Size
interdiff-1999388-24-29.txt	5.81 KB
1999388-language-request-29.patch	6.06 KB

fixed most of #27. the remaining cases seemed to use symfony request already:

language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php:479:    $request = Request::create('', 'GET', array(), array(), array(), array('HTTP
language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php:128:    $request = $this->prepareRequestForGenerator(TRUE, array('SERVER_PORT' => '88'));
language/tests/language_test/lib/Drupal/language_test/Controller/LanguageTestController.php:107:    $sub_request = Request::create($base_path . '/user', 'GET

Log in or register to post comments

Comment #30

herom CreditAttribution: herom commented 7 October 2013 at 21:05

Issue tags:

-Needs reroll

tag cleanup.

Log in or register to post comments

Comment #31

dawehner

German

CreditAttribution: dawehner commented 22 October 2013 at 16:47

Status:

Needs review

» Reviewed & tested by the community

+++ b/core/modules/language/language.negotiation.inc
@@ -496,7 +498,13 @@ function language_url_rewrite_session(&$path, &$options) {
+        $query_value = check_plain(\Drupal::request()->query->get($query_param));

We could just switch to String::checkPlain