Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Title: Remove Unused local variable $generic_tag from /core/modules/language/language.negotiation.inc » Remove Unused local variables from language module

Updating issue title

areke’s picture

Issue summary: View changes
FileSize
4.51 KB

This needed to be re-rolled, so I re-rolled it; however, I changed one thing. In the first patch, the line
list($language, $path) = language_url_split_prefix($request_path, $languages);
was changed to
list($language, ) = language_url_split_prefix($request_path, $languages);
To me, the change makes the code harder to read and understand, meaning even though the variable is unused, it serves a purpose. Because of this, I left the line as it was.

enhdless’s picture

Patch failed to apply.

TR’s picture

3: 2080427-3.patch queued for re-testing.

forbesgraham’s picture

Assigned: Unassigned » forbesgraham
xjm’s picture

Priority: Normal » Minor

Let's make sure to confirm that there aren't other unused local variables in the module as well.

parthipanramesh’s picture

Status: Needs review » Needs work

Sorry, but I can't apply this patch..

TR’s picture

Status: Needs work » Needs review

3: 2080427-3.patch queued for re-testing.

areke’s picture

Status: Needs review » Needs work

I'm setting this to needs work as there 5 more unused variables in the language module.

Unused local variable $base_url  (file .../lib/Drupal/language/Tests/LanguageConfigurationTest.php - [language] line: 37)
Unused local variable $base_url (file .../lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.php - [language] line: 37)
Unused local variable $base_url (file .../lib/Drupal/language/Tests/LanguageListTest.php - [language] line: 37)
Unused local variable $request (file .../lib/Drupal/language/Tests/LanguageUrlRewritingTest.php - [language] line: 62)
Unused local variable $mappings (file .../lib/Drupal/language/Form/NegotiationBrowserForm.php - [language] line: 140)

martin107’s picture

Comment #10 is unhelpful

The list of unused variables are already removed by 2080427-3.patch

Perhaps someone has already applied his comments.

Except for the the last one in NegotiationBrowserForm.php where the quoted variable IS used at line 143 in a foreach loop

martin107’s picture

Status: Needs work » Needs review

3: 2080427-3.patch queued for re-testing.

jsobiecki’s picture

Assigned: Unassigned » jsobiecki
jsobiecki’s picture

This patch doesn't apply. What's exactly is wrong, is a fact, that one of file modified by patch doesn't exist: /core/modules/language/language.negotiation.inc (at least at HEAD).
It was removed at 0b55dcd84190efb23e9db99e9fc135979aeec9bc (#1862202)

Old version of patch modifies not-existing function language_from_language(). This function was replaced by method LanguageNegotiationBrowser::getLangcode(). In this method, issue with unused variables doesn't exist.

Rest of patch (test files) applies cleanly and fixes problem at test files. I will try to reroll patch and remove deprecated parts of it.

jsobiecki’s picture

FileSize
3.17 KB
jsobiecki’s picture

Assigned: jsobiecki » Unassigned
blainelang’s picture

FileSize
5.4 KB
blainelang’s picture

Adding this comment on behalf of the toronto group sprint team.

We have reviewed this patch and it applied cleanly but upon review we found there were a few usages of deprecated functions,

Specifically the use of language_list which needs to be replaced by: \Drupal::languageManager()->getLanguages();
and language_load() with \Drupal::languageManager()->getLanguage()

Last update lost my comments.

Status: Needs review » Needs work

The last submitted patch, 17: issue_2080427-17.patch, failed testing.

The last submitted patch, 17: issue_2080427-17.patch, failed testing.

blainelang’s picture

FileSize
5.36 KB

The last patch failed, attached is an updated patch that just addresses un-used local variables in all the Language Test files (using phpstorm code inspect)

blainelang’s picture

Status: Needs work » Needs review

Queue for re-testing

Coy Lay’s picture

Not sure why this change was included as part of this patch:

-    // Get the count of languages.
+    // Reset all language types
Coy Lay’s picture

Not sure why this change was included as part of this patch:

-    // Get the count of languages.
+    // Reset all language types
blainelang’s picture

It looked like a comment that had come across as part of a copy paste as I didn't see any logic below that referencing or using a count.

connorwk’s picture

Here at the Austin Sprint, I'm going to be working on this issue.

connorwk’s picture

Just rerolled this patch and will continue to review and work on it.

connorwk’s picture

I reviewed the reroll it looks good.
I added to the patch to remove the other few unused variables in language.
Just needs one last review and this should be done.

Xano’s picture

Status: Needs review » Needs work

Looks good!

+ // Reset all language types
Why was this comment added? Let's take that out, and then I think this is good to go.

connorwk’s picture

Status: Needs work » Needs review
FileSize
6.57 KB
723 bytes

Ok removed it.

Xano’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 43324d1 and pushed to 8.x. Thanks!

  • Commit 43324d1 on 8.x by alexpott:
    Issue #2080427 by connork, blainelang, harijari, areke, chertzog: Remove...

Status: Fixed » Closed (fixed)

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