Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables

File /core/lib/Drupal/Core/StreamWrapper/LocalStream.php

Line 90: Unused local variable $scheme
Line 406: Unused local variable $target

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

duozersk’s picture

Assigned: duozersk » Unassigned
Status: Active » Needs review
FileSize
964 bytes
andypost’s picture

Status: Needs review » Reviewed & tested by the community

nice

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.phpundefined
@@ -87,7 +87,7 @@ protected function getTarget($uri = NULL) {
+    list( , $target) = explode('://', $uri, 2);

The list( , should be list(, as there are 14 examples of this in core and only 1 example of the other way around.

duozersk’s picture

Assigned: Unassigned » duozersk

Good, will do.

duozersk’s picture

Status: Needs work » Needs review
FileSize
963 bytes
duozersk’s picture

Assigned: duozersk » Unassigned
Status: Needs review » Reviewed & tested by the community

RTBC as it was already reviewed.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

As mentioned elsewhere, these ones that change list($foo, $bar) to list($foo, ) make the code less readable. Is there another way we can remove the error without doing that?

duozersk’s picture

Assigned: Unassigned » duozersk
FileSize
921 bytes

Not sure if it is the optimal way to do this.
Basically, used the strpos() and drupal_substr().

duozersk’s picture

Assigned: duozersk » Unassigned
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose this approach is less readable but according @webchick this is another way

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I committed #5 sometime in my recent commit spree, but apparently lost my comment!

tstoeckler’s picture

As far as I know, we can simply do list($foo) which is actually much more readable.

duozersk’s picture

Do we need a follow-up patch then? Should it be just added here or we need to create a new issue? I can do that, just need a guidance here.

tstoeckler’s picture

Status: Fixed » Needs work

In this case I think a follow-up patch would be preferred. Thanks @duozersk!

duozersk’s picture

OK, here it goes. Removed 2 symbols ", " per #12 above.

duozersk’s picture

Assigned: Unassigned » duozersk
Status: Needs work » Needs review

Waiting for the test results.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks! Patch looks great.

duozersk’s picture

Assigned: duozersk » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9a7ef5d and pushed to 8.x. Thanks!

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