[2.3] Static Code Analysis for Components #17085

kalessil · 2015-12-21T09:53:22Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

Static Code Analysis with Php Inspections (EA Extended):
- remaining mkdir race conditions
- continue miss-usage in switch

…nue miss-behaviour

EVODelavega · 2015-12-21T10:04:39Z

Please use sprintf for exception messages, instead of concatenating variables into the message:

Yes, using sprintf is what the coding standard should document. Can you
submit a PR on the docs? And perhaps a PR on symfony/symfony to fix
inlined exceptions?

Thanks,
Fabien

Also see Symfony coding standards last item under structure:

Exception message strings should be concatenated using sprintf.

linaori · 2015-12-21T12:52:53Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

-        if (!is_dir(dirname($cache))) {
-            mkdir(dirname($cache), 0777, true);
+        if (!file_exists($cacheDir) && !@mkdir($cacheDir, 0777, true) && !is_dir($cacheDir)) {
+            throw new \RuntimeException('Class Collection Loader was not able to create a folder: '.$cacheDir);


I would call it a 'directory' instead and make sure exceptions end with a full stop, same goes for the other exceptions.

Tobion · 2015-12-21T13:04:04Z

According to twigphp/Twig#1944 this is not fully safe against race conditions either. And now we might throw exceptions in placed where it worked before.

…e is_dir instead of file_exists

kalessil · 2015-12-21T15:18:02Z

@Tobion : the same as in #16292 - pattern and solution; I'm not sure if this breaks BC.

kalessil · 2015-12-21T16:53:11Z

Exceptions message generation changed in favor of sprintf

nicolas-grekas · 2015-12-22T08:54:51Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

-        if (!is_dir(dirname($cache))) {
-            mkdir(dirname($cache), 0777, true);
+        if (!is_dir($cacheDir) && !@mkdir($cacheDir, 0777, true) && !is_dir($cacheDir)) {
+            throw new \RuntimeException(sprintf('Class Collection Loader was not able to create a directory: %s', $cacheDir));


Does this add anything? If the directory doesn't exists, writeCacheFile already throws an exception...

Do you mean "Failed to write cache file"?

I'd rather expect the component to separate exceptions: because case "directory can not be created" and "file not dumped/file not renamed" needs different investigations.

…es made consistent

… consistent

nicolas-grekas · 2015-12-28T12:58:30Z

src/Symfony/Component/HttpFoundation/HeaderBag.php

@@ -52,7 +52,7 @@ public function __toString()
        foreach ($this->headers as $name => $values) {
            $name = implode('-', array_map('ucfirst', explode('-', $name)));
            foreach ($values as $value) {
-                $content .= sprintf("%-{$max}s %s\r\n", $name.':', $value);
+                $content .= sprintf("%-${max}s %s\r\n", $name.':', $value);


personally, I prefer {$max}; it generates much cleaner tokens when parsing (think e.g. static analyzers)

It were 4 more cases with sprintf and inline variables, as ${...} structure. Should I make adjustments to {$...} then?

${...} -> {$...}: done

…consistent (${...} => {$...})

nicolas-grekas · 2015-12-28T20:50:53Z

src/Symfony/Component/ClassLoader/ClassCollectionLoader.php

-        if (!is_dir(dirname($cache))) {
-            mkdir(dirname($cache), 0777, true);
+        if (!is_dir($cacheDir) && !@mkdir($cacheDir, 0777, true) && !is_dir($cacheDir)) {
+            throw new \RuntimeException(sprintf('Class Collection Loader was not able to create a directory "%s"', $cacheDir));


the "a" before "directory" should be remove (below also)

"a"s removed in all exceptions messages from this PR

nicolas-grekas · 2015-12-31T09:03:50Z

👍
Status: reviewed

xabbuh · 2015-12-31T10:35:47Z

src/Symfony/Component/ClassLoader/ClassMapGenerator.php

@@ -134,7 +134,7 @@ private static function findClasses($path)
                    }

                    if ($isClassConstant) {
-                        continue;
+                        break;


Are you sure about this change? continue relates to the for loop while break only leaves the switch.

Yes, continue inside switch behaves differently in PHP than in other languages.

http://php.net/manual/en/control-structures.continue.php

Note: In PHP the switch statement is considered a looping structure for the purposes of continue. continue behaves like break (when no arguments are passed). If a switch is inside a loop, continue 2 will continue with the next iteration of the outer loop.

oh god PHP :(

I fully understand =)

Or take in account finally-blocks introduced in 5.5 - if you get an exception inside (even handled) you can have variety of miss-behaviors of code in different PHP versions.

But at least I can warn people using my plugin that something might go wrong and they can concentrate on right things instead of long debugging hours.

xabbuh · 2015-12-31T10:50:35Z

👍

fabpot · 2015-12-31T18:24:10Z

Thank you @kalessil.

This PR was squashed before being merged into the 2.3 branch (closes #17085). Discussion ---------- [2.3] Static Code Analysis for Components | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Static Code Analysis with Php Inspections (EA Extended): - remaining mkdir race conditions - continue miss-usage in switch Commits ------- 6d303c7 [2.3] Static Code Analysis for Components

kalessil added 2 commits December 21, 2015 10:45

Php Inspections (EA Extended): remaining mkdir race conditions, conti…

24839fb

…nue miss-behaviour

Php Inspections (EA Extended): review before PR

017ba7f

carsonbot added the Status: Needs Review label Dec 21, 2015

linaori reviewed Dec 21, 2015
View reviewed changes

Php Inspections (EA Extended): use sprintf for exception messages, us…

eaada24

…e is_dir instead of file_exists

nicolas-grekas reviewed Dec 22, 2015
View reviewed changes

kalessil added 2 commits December 23, 2015 11:42

Php Inspections (EA Extended): continue 2 -> break, exceptions messag…

4bdc26b

…es made consistent

Php Inspections (EA Extended): correct exceptions messages

e39154e

aitboudad mentioned this pull request Dec 24, 2015

Translation:Update should throw warning if directory is missing #17128

Closed

kalessil added 2 commits December 25, 2015 20:47

Php Inspections (EA Extended): make sprintf pattern (inline variable)…

c3a4f7f

… consistent

Php Inspections (EA Extended): make sprintf pattern (inline variable)…

0016474

… consistent

nicolas-grekas reviewed Dec 28, 2015
View reviewed changes

Php Inspections (EA Extended): make sprintf pattern inline variables …

918a018

…consistent (${...} => {$...})

nicolas-grekas reviewed Dec 28, 2015
View reviewed changes

Php Inspections (EA Extended): exceptions wording - "a"s removed

18ce233

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 31, 2015

xabbuh reviewed Dec 31, 2015
View reviewed changes

fabpot closed this Dec 31, 2015

Uh oh!

[2.3] Static Code Analysis for Components #17085

[2.3] Static Code Analysis for Components #17085

Uh oh!

Conversation

kalessil commented Dec 21, 2015

Uh oh!

EVODelavega commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Dec 21, 2015

Uh oh!

kalessil commented Dec 21, 2015

Uh oh!

kalessil commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Dec 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Dec 31, 2015

Uh oh!

fabpot commented Dec 31, 2015

Uh oh!

Uh oh!