Static Code Analysis with Php Inspections fixes #1377

gulaandrij · 2017-05-29T09:41:21Z

No description provided.

…orks faster than invoking functions in PHP.

…rstr(...)/strpos() for better performance because matched patterns does not contain any characters.

…andling logic behind as well. Using array access is a preferred way, because you have to implicitly write (read as document) errors handling code.

…f variable function call. Using variable function instead is a micro-optimization.

…nt do not match the ones in respective function or method declaration.

…. Prefix usage aids in identifying the source of the ID within the code and the more_entropy parameter improves randomness.

dereuromark · 2017-05-29T09:51:57Z

src/Propel/Common/Config/ConfigurationManager.php

+     *                         the constructor to pass a built-in array of configuration, without load it from file.
+     *                         I.e. Propel\Generator\Config\QuickGeneratorConfig class.
+     *
+     * @return null


dereuromark · 2017-05-29T09:52:33Z

src/Propel/Common/Config/FileLocator.php

@@ -25,7 +25,9 @@ class FileLocator extends BaseFileLocator
     * By default, the locator looks for configuration file in the current directory (where bin/propel script is running)
     * or in a 'conf' or 'config' subdirectory.
     *
-     * @param null array $configDirectories The directories list where to look for configuration file(s)
+     * @param null $configDirectories


null itself can never be valid. There must always be a 2nd param type.
probably

array|null

dereuromark · 2017-05-29T09:54:45Z

src/Propel/Common/Config/FileLocator.php

-     * @param null array $configDirectories The directories list where to look for configuration file(s)
+     * @param null $configDirectories
+     *
+     * @internal param array $null $configDirectories The directories list where to look for configuration file(s)


can then be removed

dereuromark · 2017-05-29T09:55:29Z

src/Propel/Common/Config/Loader/FileLoader.php

@@ -61,6 +61,8 @@ public function __construct(FileLocatorInterface $locator = null)
     * Replaces parameter placeholders (%name%) by their values for all parameters.
     *
     * @param array $configuration The configuration array to resolve
+     *
+     * @return array|void


...|void is not a thing
Use ...|null instead.

Well, void is a thing. It's used when the function does not return anything. So its ok to have it here.

Not together with with another type. That by its very core definition does not exist :)
See http://www.dereuromark.de/2015/10/05/return-null-vs-return-void/

PHP7 has a void return type in its core, so it's semantically helpful to say this function returns an array or simply something insignificant. Array or null is something different and would imply that null is significant where void says pretty much the opposite - nothing new for you.

Since this function does not actually return anything, void is perfectly correctly placed here.

A question that can be asked later is why this function returns void but that's out of scope of this PR. This PR is about making the doc and other stuff coherent with coding standard and technically correct annotations. This annotation is correctly placed so should stay that way. Changing that and its code now means changing the purpose of this PR - which should be avoided at all costs, considering the diff is already huge.

You are wrong about this.
Void means that this method cannot by definition return anything, you "may" not check on the return result, as it makes no sense.
So if something returns anything not null, then it always must always return null then (instead of void) as alternative. Because then you actually do check on the return type and !== null etc.
As the blog post clearly explains, there is no something|void from a logical stand point.
PHP7 typehinting you mentions actually backs this up to 100%, but this is already something done for many many years now - on a pure semantic level of course.
PHP7 here is more the result of those many years of semantic only becoming a implemented code piece now. Which is a good thing.

I still disagree on that and what you claim in the blog post. Semantically, you can not just replace void by null just because the function has a state where it can return something not null|void'ish. The interpretation of array|void is different to array|null since later would imply null is signification as its an actual type. That is completely independent of the fact that you check the result type.

However, one thing is for sure: Changing that annotation means changing the function behaviour and its interpretation, which is out of scope of this PR. So, technically, its completely fine to place void here, since return; is void. Whether that makes sense in combination with another return type is a question you can ask later on - although I'd still disagree on your interpretation of what void means in combination with a real type.

imply null is signification as its an actual type

But it IS significant in its type - when checking on the return type there is literal NULL you will get back. For that code that checks the return type it is of absolute irrelevance how that null came there, all that matters is that there can be one returned then.
You are arguing from exactly the wrong direction.
Return types matter for the code that has to use it, not the actual method. At least for the scope we are discussing right now.
By saying void you say that this method cannot return something, but then you also say array is returned, which contradicts the previous statements. those cannot exist together, as outlined now many times before. If you start returning sth, you can only make it nullable, but never ever void again.

As to this PR and the scope: Of course I agree - as mentioned above.

Please also note that PHPStorm and alike can work with what I recommend, and they outline with yellow misuse of this. As a developer you get earlier feedback about these issues. Let the IDE help you where possible. In recent versions the IDEs understand exactly this and both for the method itself and for the code using it will help to provide exactly what I outlined above.

As already said, I disagree on your interpretation on that. Let's move on with the actual purpose of this PR.

dereuromark · 2017-05-29T09:56:08Z

src/Propel/Generator/Behavior/Archivable/ArchivableBehaviorObjectBuilderModifier.php

@@ -89,6 +95,8 @@ public function postUpdate($builder)
     * The actual deletion is made by the query object, so the AR class must tell
     * the query class to enable or disable archiveOnDelete.
     *
+     * @param $builder


Type missing, also below.

dereuromark · 2017-05-29T09:58:22Z

src/Propel/Generator/Schema/Dumper/XmlDumper.php

@@ -503,8 +503,10 @@ private function appendIndexNode(Index $index, \DOMNode $parentNode)
    /**
     * Appends the generated <unique> XML node to its parent node.
     *
-     * @param Unique   $unique     The Unique model instance
+     * @param Unique   $index


All object types should be FQCN, but this could be outside of this PR scope, maybe a follow up.

Not necessary to have FQCN everywhere. Only when its ambiguous.

It is a common practice for all library and framework code in PHP to have FQCN here always, that will get rid of use statements that are only included to make doc blocks (semantics). Amongst other advantages. There are no disadvantages, there are sniffs to auto-fix this (see spryker one) as well as IDEs like PHPStorm to also do that on its own.

But as I said: Out of scope now of course.
The changes are already getting a bit too much.

No, it's not common practice. I prefer as many others to have it not unnecessary verbose. So please don't introduce now full class names everywhere.

What is the purpose of a PR like this then? And what are the changes of it being accepted?
It also seems to comply with a lot of "best practice" approaches so far established in PHP.
FQCN doc blocks is just one of many.

What is the purpose of a PR like this then? And what are the changes of it being accepted?

See the title "Static Code Analysis with Php Inspections fixes". As far as I saw, all inspections are correct in terms of "we as propel accept on that", but I'm not done looking through all changes yet. However, a inspection that says "FQCN is mandatory for @param and @return" is not something I'd agree.

It also seems to comply with a lot of "best practice" approaches so far established in PHP.

Well, its your opinion that it is best practices - I bet I find enough people that have a different opinion on that. Stating its best practice in "whole PHP" is a very bold assumption I doubt you can prove, so I'd be very careful to play the best practice card. If you can show me a statistics that shows that most libraries use FQDN everywhere then we can talk about that.

gulaandrij · 2017-05-29T11:28:33Z

Can someone check sqlite errors?

…rator".

…ull(...)' calls can be safely replaced with 'null === ...' constructs (or 'null !== ...' if the original construct was negated).

…n applied to a variable sequentially.

marcj · 2017-05-30T12:43:40Z

src/Propel/Generator/Model/Database.php

@@ -842,7 +842,7 @@ public function __toString()
            foreach ($table->getColumns() as $column) {

                $columns[] = sprintf("      %s %s %s %s %s %s %s",
-                    $column->getName(),
+                    $column->getName(), 


use it for restart travis checks, it some times get false-positive errors, like that dc24753

dereuromark · 2020-05-06T09:55:47Z

Maybe this can be re-done in smaller chunks and then applied piece by piece with easier review.
Once those are in, we can start setting phpstan levels 1,2,3 etc.

gulaandrij added 30 commits May 29, 2017 10:59

remove unnecessary implements

6cced7b

Case Mismatched

95894a5

Dynamic method invocation via '::'

bb2fc59

Use boolean

1adce5c

iterator replase

1f3bcea

use short syntax

4c17985

static method invocation via '->'

9a875f0

Unnecessary double quotes

12bdca8

Unnecessary fully qualified name

8047e71

Unnecessary parentheses

4da28e1

Unnecessary semicolon

2f233ea

Unnecessary usage the silence operator (@)

c8fc332

Use in array

04808a5

Simplify return

12c762e

Remove one-time-use variables

5e07ff1

Remove unneeded 'else'

2cc5a70

String normalization

873400c

'switch' to 'if'

ecb52d5

Simplify ternary operator

91f703a

Merge unset

54f56c0

Don't use aliases of function

d584ab9

Use elfise operator

bd2ebd6

Use null comparation

3887227

Use type casting

945b6ac

Reports array_push(...) calls behaving as '$array []= ...'. ' []= ' w…

dac7c9a

…orks faster than invoking functions in PHP.

Reports misused in_array(...) calls.

9f67f07

Reports if strstr(...) is being used as strpos(...).

567f0bb

Reported cases can be refactored by using case-sensitive functions st…

6635327

…rstr(...)/strpos() for better performance because matched patterns does not contain any characters.

A 'substr()' call can behave as array access, but hiding some error-h…

aff77e7

…andling logic behind as well. Using array access is a preferred way, because you have to implicitly write (read as document) errors handling code.

Some of call_user_func()/call_user_func_array() might have semantic o…

362caaa

…f variable function call. Using variable function instead is a micro-optimization.

gulaandrij added 3 commits May 29, 2017 12:23

Function's or method's PHPDoc comment doesn't have a @return tag.

0243b2a

A number of parameters, their names or types (if any) in PHPDoc comme…

68344f3

…nt do not match the ones in respective function or method declaration.

Analyzes uniqid calls and reports if both parameters are not provided…

c66836e

…. Prefix usage aids in identifying the source of the ID within the code and the more_entropy parameter improves randomness.

dereuromark reviewed May 29, 2017

View reviewed changes

gulaandrij added 2 commits May 29, 2017 13:08

fix reviews

9e93c43

fix review

a734ea5

gulaandrij added 10 commits May 29, 2017 14:34

optimize imports

41a683b

Reports if it's possible to simplify ternary operator with "Elvis ope…

108c66a

…rator".

Reports 'is_null(...)' usage, a feature introduced in PHP4. All 'is_n…

0af090a

…ull(...)' calls can be safely replaced with 'null === ...' constructs (or 'null !== ...' if the original construct was negated).

Analyzes str_replace(...) calls and reports when the function has bee…

636e141

…n applied to a variable sequentially.

fix sprintf param

aa09548

try to fix sqlite tests

c4b064a

try to fix sqlite tests2

71c4a22

try to fix sqlite tests3

7394075

Merge branch 'master' into master

dc24753

recheck

b6fac96

marcj reviewed May 30, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static Code Analysis with Php Inspections fixes #1377

Static Code Analysis with Php Inspections fixes #1377

gulaandrij commented May 29, 2017

dereuromark May 29, 2017

dereuromark May 29, 2017 •

edited

Loading

dereuromark May 29, 2017

dereuromark May 29, 2017 •

edited

Loading

marcj May 29, 2017

dereuromark May 29, 2017

marcj May 29, 2017 •

edited

Loading

dereuromark May 29, 2017 •

edited

Loading

marcj May 29, 2017 •

edited

Loading

dereuromark May 29, 2017 •

edited

Loading

dereuromark May 29, 2017

marcj May 29, 2017

dereuromark May 29, 2017

dereuromark May 29, 2017

marcj May 29, 2017

dereuromark May 29, 2017 •

edited

Loading

dereuromark May 29, 2017

marcj May 29, 2017

dereuromark May 29, 2017

marcj May 29, 2017 •

edited

Loading

gulaandrij commented May 29, 2017

marcj May 30, 2017

gulaandrij May 30, 2017

dereuromark commented May 6, 2020

Static Code Analysis with Php Inspections fixes #1377

Are you sure you want to change the base?

Static Code Analysis with Php Inspections fixes #1377

Conversation

gulaandrij commented May 29, 2017

Choose a reason for hiding this comment

dereuromark May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcj May 29, 2017 • edited Loading

Choose a reason for hiding this comment

dereuromark May 29, 2017 • edited Loading

Choose a reason for hiding this comment

marcj May 29, 2017 • edited Loading

Choose a reason for hiding this comment

dereuromark May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcj May 29, 2017 • edited Loading

Choose a reason for hiding this comment

gulaandrij commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereuromark commented May 6, 2020

dereuromark May 29, 2017 •

edited

Loading

dereuromark May 29, 2017 •

edited

Loading

marcj May 29, 2017 •

edited

Loading

dereuromark May 29, 2017 •

edited

Loading

marcj May 29, 2017 •

edited

Loading

dereuromark May 29, 2017 •

edited

Loading

dereuromark May 29, 2017 •

edited

Loading

marcj May 29, 2017 •

edited

Loading