Add SpecifyArgSeparatorFixer to make sure http_build_query is properly used #18

OndraM · 2018-05-07T17:37:57Z

Fixes #17 .

To be honest, this is the first Fixer I've ever written. So any suggestions especially regarding usage of php-cs-fixer internals (the tokens stuff etc.) are welcome. Also I may overlook some simpler way how to accomplish this, so I'm open for suggestions.

Cc @TomasVotruba - I would be grateful to hear some feedback on the Fixer from you as well ❤️ :-)

TomasVotruba

Very nice work on first Fixer. I wish mine was to simple and easy to understand :D

Since you don't use pure PHP CS Fixer, I have one more tip: I do use ECS Tester package, to simulate 1:1 ECS behavior (less WTF, as it's the same as final code)

TomasVotruba · 2018-05-07T19:29:52Z

composer.json

+    },
+    "autoload-dev": {
+        "psr-4": {
+            "Lmc\\CodingStandard\\": "tests/"


Why not convetion "Lmc\\CodingStandard\\Tests\\": "tests/"?

We use the same namespace for Class and its corresponding test.

I know :D but why not convention?

AFAIK both of the approaches are widely used, so I don't consider any of them as "general convention".

And why it should be in standalone namespace in the first place? I don't see a reason for this.

The same namespace means you don't need to import the class under test namespace, also it makes sense to me to have class and its tests in the same "module" (namespace), because they belongs to each another.

But its still more about personal preference...

Similar unconvention as putting src and tests in the same directory, causes in php cs fixer only troubles. 80 % of php-scoper setup took me only this to solve.

It's really painful to work with such packages, where maintainer don't see behing the package common use.

Sorry, I don't understand :). This is the way we use it for years and never heard of single issue. And it is not any "hack", just usual setting of Composer autoloading. But yeah, I can imagine this could cause trouble when developing some low-level stuff.

BTW src/ and tests/ in the same (root) directory are used almost everywhere except packages which are then splited to sub-packages (like Symfony). I find most inconvenient when source is not in standalone directory and mixed with Tests directory on the same level. Setting code-coverage or phpcs etc. blacklist/whitelist is nightmare in this case.

Again, open source code is very different to closed one.

True, but I don't think the difference is in the way autoloading is set up :). But this seems like a topic to pub/meetup, so let's don't forget it next time we meet :-D . I'd like to hear about the stuff you must dealt with :).

TomasVotruba · 2018-05-07T19:31:27Z

src/Fixer/SpecifyArgSeparatorFixer.php

+            . '`$arg_separator` in third parameter of the function.',
+            null,
+            null,
+            'Risky when other than default "&" argument separator should be used in query strings.'


I'd expect this line rather (even) at isRisky() method

TomasVotruba · 2018-05-07T19:32:31Z

src/Fixer/SpecifyArgSeparatorFixer.php

+                    && ($tokens[$previousIndex]->isGivenKind(CT::T_FUNCTION_IMPORT)
+                        || $tokens[$previousIndex]->isGivenKind(T_FUNCTION))) {
+                    return;
+                }


I'd add all this to "shouldSkip()" method, it's not relevant to fixer process at all

TomasVotruba · 2018-05-07T19:33:29Z

src/Fixer/SpecifyArgSeparatorFixer.php

+
+    public function getName(): string
+    {
+        return mb_strtolower(self::class);


Why did you choose this new approach?

I'd use one of standards: php-cs-fixer or ECS. How could you copy this class to easy-coding-standard.yml from CLI output to skip it?

"The name must be all lowercase and without any spaces." - this is in phpDoc.
But I saw you use class name. So I combined them both :-D. What is the best solution? Where is this name being used?

ECS is depending just on the class name FQCN, or it somehow works with this?

TomasVotruba · 2018-05-07T19:33:51Z

src/Fixer/SpecifyArgSeparatorFixer.php

+        $startBraceIndex = $tokens->getNextTokenOfKind($functionIndex, ['(']);
+        if ($startBraceIndex === null) {
+            return;
+        }


Why is this here? Should be above the fix funciton

getNextTokenOfKind() can by annotation return null as well. So to adhere type safety, this case must be somehow handled.

TomasVotruba · 2018-05-07T19:35:26Z

src/Fixer/SpecifyArgSeparatorFixer.php

+
+                continue;
+            }
+        }


Might be helpful:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.11/src/Tokenizer/Analyzer/ArgumentsAnalyzer.php

I'd be happy to check after that, there is lot of outsourcable code now

:-D I was today looking for something like this like an half an hour and did not find it. So this makes part of the code unnecessary, I will refactor it with use of the ArgumentsAnalyzer. Thanks!

TomasVotruba · 2018-05-07T19:36:23Z

src/Fixer/SpecifyArgSeparatorFixer.php

+            $thirdParamToken = $tokens[$thirdParamTokenIndex];
+
+            if ($thirdParamToken->isGivenKind(T_STRING) && $thirdParamToken->getContent() === 'null') {
+                $tokens->offsetSet($thirdParamTokenIndex, $ampToken);


This section very difficult to read. What does it do?

I'd expect it to be bellow, with other "modifying" code

TomasVotruba · 2018-05-07T19:37:41Z

src/Fixer/SpecifyArgSeparatorFixer.php

+            $tokensToInsert[] = new Token([T_STRING, 'null']);
+        }
+
+        // Add third parameter (arg separator)


Real code might be also helfpul while reading this, e.g.:

// Add third parameter (arg separator): ", &")

I always add it because it's much more readable than custom Tokens

TomasVotruba · 2018-05-07T19:38:36Z

tests/Fixer/SpecifyArgSeparatorFixerTest.php

+        $fixer->fix($fileInfo, $tokens);
+
+        $this->assertSame(
+            file_get_contents(__DIR__ . '/Fixtures/' . $expectedOutputFile),


asssertStringEqualsFile() or how its called

TomasVotruba · 2018-05-07T19:39:18Z

tests/Fixer/SpecifyArgSeparatorFixerTest.php

+        return [
+            'Correct file should not be changed' => ['Correct.txt', 'Correct.txt'],
+            'Wrong file should be fixed' => ['Wrong.txt', 'Fixed.txt'],
+        ];


While using dataProvider, it makes sense to use one file per case.
Imagine you have bug in Wrong.txt on line 25 - which one of these cases it?

@TomasVotruba True, but there is overhead and is also harder to see what is covered and what not. Maybe rather write inline PHP for each case (as some fixer tests do).

You will see which line of code is broken by phpunit's diff, right?

Well, how do you run broken code only?

You dont (I guess).. but why would you run just part of tests?
If you are developing something new, you can run single test case or I think even one perticular provider value test case in PHPStorm..

I do that too, but how to run specific one line in this single file?

MortalFlesh · 2018-05-09T07:40:23Z

.travis.yml

  - travis_retry composer update --no-interaction

 script:
-  - composer all


why not to encapsulate those commands into all script?

I need phpunit with specific settings for CI (to generate clover). But I may do something like "all-ci" or so.

MortalFlesh · 2018-05-09T07:42:20Z

composer.json

    "require-dev": {
        "j13k/yaml-lint": "^1.1",
-        "mhujer/yaml-sort-checker": "^1.2"
+        "mhujer/yaml-sort-checker": "^1.2",


Did you consider using lmc/coding-standards here as well?

Why :)? This is the package itself 🙃

MortalFlesh · 2018-05-09T07:50:02Z

src/Fixer/SpecifyArgSeparatorFixer.php

+        foreach ($tokens as $index => $token) {
+            if ($token->isGivenKind(T_STRING) && $token->getContent() === 'http_build_query') {
+                if ($this->isNotFunctionCall($tokens, $index)) {
+                    return;


Are you sure with return? IMHO this should be just continue, or am I wrong?

Imagine code

function foo($args) { $this->addCallback('http_build_query'); // this is not a function call return http_build_query($args); // this is a function call, but isn't it skipped due previous `return`? }

Nice catch, fixed. 👍

Tests actually hide this, because the case with definition of function http_build_query were the last, so the bug did not show :). So I rearranged them.

MortalFlesh · 2018-05-09T07:52:31Z

src/Fixer/SpecifyArgSeparatorFixer.php

+        }
+
+        $endBraceIndex = $tokens->findBlockEnd(Tokens::BLOCK_TYPE_PARENTHESIS_BRACE, $startBraceIndex);
+        $ampToken = new Token([T_STRING, "'&'"]);


Well, would this Token find usage of http_build_query(..., "&");? You explicitely awaits it is in single quotes.

Good point. Priority is now set to -1, ie. it will run after SingleQuoteFixer.

MortalFlesh · 2018-05-09T07:54:03Z

src/Fixer/SpecifyArgSeparatorFixer.php

+
+        $commaIndexes = [];
+
+        for ($index = $startBraceIndex + 1; $index < $endBraceIndex; ++$index) {


Just saying.. but this looks very confusing to me (+ 1, ++$index,...).

MortalFlesh · 2018-05-09T07:56:41Z

src/Fixer/SpecifyArgSeparatorFixer.php

+            $token = $tokens[$index];
+
+            // Parameter surrounded by brace, skip to its end
+            if ($token->equals('(')) {


mm.. I guess following code is a nightmare for Fixer author, but have you considered something like this?

http_build_query([], null, ('&'));

OndraM · 2018-05-10T14:52:31Z

Changes done & refactored to use ArgumentsAnalyzer. cc @TomasVotruba @MortalFlesh et al. 🙏

TomasVotruba · 2018-05-10T16:42:46Z

In which commit did you add the ArgumentAnalyzer? The commit messages are not very useful :(

MortalFlesh · 2018-05-11T08:55:51Z

src/Fixer/SpecifyArgSeparatorFixer.php

+    /**
+     * Return false if this is function import or function definition (and not function call)
+     */
+    private function isNotFunctionCall(Tokens $tokens, int $index): bool


I'm not fan of negative bool methods or variables, it is confusing to use/read .. I'd prefer function isFunctionCall()
and then if (!$this->isFunctionCall())..

Neither am I, but this function in fact doesn't prove it is function call when you just negate the logic - there are IMHO many edge cases. What it can is just to detect the main scenarios when it is not a function call :). So from my POV, the other name would be inaccurate and misleading :).
🤷

Well.. 🤔
Ok in this case I think it might be ok..

@MortalFlesh I thought about it again and changed it in the end... The objection still applies, but maybe its less harm like this 🤷‍♂️ .

MortalFlesh · 2018-05-11T08:56:21Z

src/Fixer/SpecifyArgSeparatorFixer.php

+
+    private function getThirdArgumentInfo(
+        Tokens $tokens,
+        $openParenthesisIndex,


Why there is no type?

How can this pass the CI?

Fixed.

@TomasVotruba It seems phpdoc_add_missing_param_annotation (aka PhpdocAddMissingParamAnnotationFixer) does not work as I expected :-(. It only works if there is already at least something in phpdoc.

Do you know some check which takes care of this?

MortalFlesh · 2018-05-11T08:57:40Z

src/Fixer/SpecifyArgSeparatorFixer.php

+        $argumentsAnalyzer = new ArgumentsAnalyzer();
+
+        $arguments = $argumentsAnalyzer->getArguments($tokens, $openParenthesisIndex, $closeParenthesisIndex);
+        $argumentIndex = array_slice($arguments, 2, 1, true);


Well I would name it rather $argument, instead of $argumentIndex, which I'd expect holds just an index of the argument.
And maybe name it explicitely $thirdArgument..

So is this int (index/position) or some array/object ($argument) ?

The content is [startIndex => endIndex]. IMO argument is still misleading - it contains just the indexes, not the argument itself 🤔

I name these BlockInfo

OndraM · 2018-05-11T09:55:12Z

@TomasVotruba

In which commit did you add the ArgumentAnalyzer? The commit messages are not very useful :(

You can easily go to Files changed > Changes from: > Select "Since last review" or something like this (it is selected by default) to see what has changed.

We don't write new commit messages when you only adjust things in response to code-review feedback, because commit messages should be about behavior change in the code, to make the history help with long term maintainability of the code (it helps a lot when using git blame etc.). There already is commit about adding the fixer, so unless any new commit in this PR will add/change some other behavior, the changes are just associated to the original commit via fixup and will be rebased and squashed before merge.

Interesting article about this topic: https://arialdomartini.wordpress.com/2012/09/03/pre-emptive-commit-comments/ (especially "Tell me what the software does (not what you did)" part) and also of course https://filip-prochazka.com/blog/git-fixup about fixup commits.

TomasVotruba · 2018-05-11T10:27:50Z

It's just uncommon and confusing in open source, that's all.

…ined in http_build_query() (fixes #17)

…des?

OndraM · 2018-05-14T15:07:23Z

Merged, thanks @TomasVotruba @MortalFlesh a lot for your great feedback! 🥇

OndraM added the enhancement New feature or request label May 7, 2018

OndraM requested review from MortalFlesh, jirinovak, kubasimon and legendik May 7, 2018 17:37

TomasVotruba reviewed May 7, 2018

View reviewed changes

OndraM force-pushed the feature/arg-separator branch from 8f1eca0 to bfbdc36 Compare May 7, 2018 21:02

MortalFlesh requested changes May 9, 2018

View reviewed changes

MortalFlesh requested changes May 11, 2018

View reviewed changes

MortalFlesh approved these changes May 14, 2018

View reviewed changes

OndraM added 2 commits May 14, 2018 17:02

Add SpecifyArgSeparatorFixer to make sure arg_separator is always def…

c65ecc0

…ined in http_build_query() (fixes #17)

Setup codestyle check for codestyle check. Quis custodiet ipsos custo…

c834dd8

…des?

OndraM force-pushed the feature/arg-separator branch from 77b31af to c834dd8 Compare May 14, 2018 15:02

OndraM merged commit ebd6aec into alma-oss:master May 14, 2018

OndraM deleted the feature/arg-separator branch May 14, 2018 15:07


		$commaIndexes = [];

		for ($index = $startBraceIndex + 1; $index < $endBraceIndex; ++$index) {

Add SpecifyArgSeparatorFixer to make sure http_build_query is properly used #18

Add SpecifyArgSeparatorFixer to make sure http_build_query is properly used #18

Uh oh!

Conversation

OndraM commented May 7, 2018

Uh oh!

TomasVotruba left a comment

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!

OndraM May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

OndraM May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

OndraM May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

OndraM May 11, 2018 •

edited

Loading

OndraM May 10, 2018 •

edited

Loading

OndraM May 10, 2018 •

edited

Loading

TomasVotruba commented May 10, 2018 •

edited

Loading

OndraM May 11, 2018 •

edited

Loading