Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables
File /core/modules/language/language.negotiation.inc
Line 132: Unused local variable $generic_tag
Line 290: Unused local variable $path
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff-2080427-28-30.txt | 723 bytes | connorwk |
#30 | remove-unused-2080427-30.patch | 6.57 KB | connorwk |
#28 | interdiff-2080427-27-28.txt | 2.65 KB | connorwk |
#28 | remove-unused-2080427-28.patch | 6.6 KB | connorwk |
#27 | remove-unused-2080427-27.patch | 4.69 KB | connorwk |
Comments
Comment #1
chertzogCombining some of the files to trim number of patches.
Closing the following as dupilcates:
#2080431: Remove Unused local variable $request from /core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php
#2080433: Remove Unused local variable $base_url from /core/modules/language/lib/Drupal/language/Tests/LanguageListTest.php
#2080437: Remove Unused local variable $base_url from /core/modules/language/lib/Drupal/language/Tests/LanguageConfigurationTest.php
#2080441: Remove Unused local variable $base_url from language/lib/Drupal/language/Tests/LanguageCustomLanguageConfigurationTest.php
Comment #2
chertzogUpdating issue title
Comment #3
areke CreditAttribution: areke commentedThis 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.
Comment #4
enhdless CreditAttribution: enhdless commentedPatch failed to apply.
Comment #5
TR CreditAttribution: TR commented3: 2080427-3.patch queued for re-testing.
Comment #6
forbesgraham CreditAttribution: forbesgraham commentedComment #7
xjmLet's make sure to confirm that there aren't other unused local variables in the module as well.
Comment #8
parthipanramesh CreditAttribution: parthipanramesh commentedSorry, but I can't apply this patch..
Comment #9
TR CreditAttribution: TR commented3: 2080427-3.patch queued for re-testing.
Comment #10
areke CreditAttribution: areke commentedI'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)
Comment #11
martin107 CreditAttribution: martin107 commentedComment #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
Comment #12
martin107 CreditAttribution: martin107 commented3: 2080427-3.patch queued for re-testing.
Comment #13
jsobiecki CreditAttribution: jsobiecki commentedComment #14
jsobiecki CreditAttribution: jsobiecki commentedThis 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.
Comment #15
jsobiecki CreditAttribution: jsobiecki commentedComment #16
jsobiecki CreditAttribution: jsobiecki commentedComment #17
blainelang CreditAttribution: blainelang commentedComment #18
blainelang CreditAttribution: blainelang commentedAdding 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.
Comment #21
blainelang CreditAttribution: blainelang commentedThe 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)
Comment #22
blainelang CreditAttribution: blainelang commentedQueue for re-testing
Comment #23
Coy Lay CreditAttribution: Coy Lay commentedNot sure why this change was included as part of this patch:
Comment #24
Coy Lay CreditAttribution: Coy Lay commentedNot sure why this change was included as part of this patch:
Comment #25
blainelang CreditAttribution: blainelang commentedIt 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.
Comment #26
connorwk CreditAttribution: connorwk commentedHere at the Austin Sprint, I'm going to be working on this issue.
Comment #27
connorwk CreditAttribution: connorwk commentedJust rerolled this patch and will continue to review and work on it.
Comment #28
connorwk CreditAttribution: connorwk commentedI 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.
Comment #29
XanoLooks good!
+ // Reset all language types
Why was this comment added? Let's take that out, and then I think this is good to go.
Comment #30
connorwk CreditAttribution: connorwk commentedOk removed it.
Comment #31
XanoComment #32
alexpottCommitted 43324d1 and pushed to 8.x. Thanks!