Add SpecifyArgSeparatorFixer to make sure http_build_query is properly used

.coveralls.yml

@@ -0,0 +1,2 @@
+coverage_clover: coverage-clover.xml
+json_path: coveralls-upload.json

.travis.yml

@@ -1,11 +1,15 @@
 language: php
 
 php:
+  - 7.1
   - 7.2
 
 install:
   - travis_retry composer self-update && composer --version
   - travis_retry composer update --no-interaction
 
 script:
-  - composer all
+  - composer all-ci
+
+after_success:
+  - travis_retry php vendor/bin/php-coveralls -v

CHANGELOG.md

@@ -5,6 +5,7 @@
 <!-- There is always Unreleased section on the top. Subsections (Added, Changed, Fixed, Removed) should be added as needed. -->
 
 ## Unreleased
+- Add `SpecifyArgSeparatorFixer` to make sure arg_separator is always defined when using `http_build_query()` function.
 - Add PHPUnit fixers:
     - `PhpUnitMockFixer`: Ensure dedicated helper methods `createMock()` and `createPartialMock()` are used where possible instead of `->getMock()`.
     - `PhpUnitNoExpectationAnnotationFixer`: Use `setExpectedException()` instead of `@expectedException` annotation.

composer.json

@@ -10,20 +10,53 @@
             "homepage": "https://github.com/lmc-eu"
         }
     ],
+    "autoload": {
+        "psr-4": {
+            "Lmc\\CodingStandard\\": "src/"
+        }
+    },
+    "autoload-dev": {
+        "psr-4": {
+            "Lmc\\CodingStandard\\": "tests/"
+        }
+    },
     "require": {
         "symplify/easy-coding-standard": "^4.0.2"
     },
     "require-dev": {
         "j13k/yaml-lint": "^1.1",
-        "mhujer/yaml-sort-checker": "^1.2"
+        "mhujer/yaml-sort-checker": "^1.2",
+        "phpstan/phpstan-phpunit": "^0.9.4",
+        "phpstan/phpstan-shim": "^0.9.2",
+        "phpunit/phpunit": "^7.1"
     },
     "scripts": {
         "all": [
-            "@lint"
+            "@lint",
+            "@analyze",
+            "@test"
+        ],
+        "all-ci": [
+            "@lint",
+            "@analyze",
+            "@test-ci"
         ],
         "lint": [
             "for FILE_NAME in *.yml *.yaml; do vendor/bin/yaml-lint \"$FILE_NAME\"; done",
             "vendor/bin/yaml-sort-checker"
+        ],
+        "analyze": [
+            "vendor/bin/ecs check src/ tests/ -vvv --ansi",
+            "vendor/bin/phpstan.phar analyze ./src ./tests -c phpstan.neon --level 7 --ansi"
+        ],
+        "test": [
+            "./vendor/bin/phpunit --colors=always"
+        ],
+        "test-ci": [
+            "./vendor/bin/phpunit --colors=always --coverage-clover coverage-clover.xml"
         ]
+    },
+    "config": {
+        "sort-packages": true
     }
 }

easy-coding-standard.yaml

@@ -2,6 +2,9 @@ imports:
     - { resource: '%vendor_dir%/symplify/easy-coding-standard/config/psr2.yml' }
 
 services:
+    #  Function http_build_query() should always have specified `$arg_separator` parameter
+    Lmc\CodingStandard\Fixer\SpecifyArgSeparatorFixer: ~
+
     # Class and Interface names should be unique in a project, they should never be duplicated
     PHP_CodeSniffer\Standards\Generic\Sniffs\Classes\DuplicateClassNameSniff: ~
     # Control Structures must have at least one statement inside of the body (empty catch rules is skipped)

phpstan.neon

@@ -0,0 +1,2 @@
+includes:
+	- vendor/phpstan/phpstan-phpunit/extension.neon

phpunit.xml.dist

@@ -0,0 +1,23 @@
+<phpunit
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
+    bootstrap="./vendor/autoload.php"
+    colors="true"
+>
+    <testsuites>
+        <testsuite name="unit">
+            <directory>./tests/</directory>
+        </testsuite>
+    </testsuites>
+
+    <filter>
+        <whitelist>
+            <directory suffix=".php">./src</directory>
+        </whitelist>
+    </filter>
+
+    <php>
+        <!-- E_ALL = 30719 -->
+        <ini name="error_reporting" value="30719"/>
+    </php>
+</phpunit>

src/Fixer/SpecifyArgSeparatorFixer.php

@@ -0,0 +1,152 @@
+<?php declare(strict_types=1);
+
+namespace Lmc\CodingStandard\Fixer;
+
+use PhpCsFixer\Fixer\DefinedFixerInterface;
+use PhpCsFixer\FixerDefinition\CodeSample;
+use PhpCsFixer\FixerDefinition\FixerDefinition;
+use PhpCsFixer\FixerDefinition\FixerDefinitionInterface;
+use PhpCsFixer\Tokenizer\Analyzer\Analysis\TypeAnalysis;
+use PhpCsFixer\Tokenizer\Analyzer\ArgumentsAnalyzer;
+use PhpCsFixer\Tokenizer\CT;
+use PhpCsFixer\Tokenizer\Token;
+use PhpCsFixer\Tokenizer\Tokens;
+
+class SpecifyArgSeparatorFixer implements DefinedFixerInterface
+{
+    public function getDefinition(): FixerDefinitionInterface
+    {
+        return new FixerDefinition(
+            'Function `http_build_query()` should always have specified `$arg_separator` argument',
+            [
+                new CodeSample("<?php\n\$queryString = http_build_query(['foo' => 'bar', 'baz' => 'bat']);"),
+            ],
+            'Function `http_build_query()` uses `arg_separator.output` ini directive as default argument separator, '
+            . 'however when its default value "&" is changed, query string assembled by the method will be '
+            . 'unexpectedly invalid. This Fixer forces you to not rely on ini settings and rather define '
+            . '`$arg_separator` in third argument of the function.',
+            null,
+            null,
+            'Risky when other than default "&" argument separator should be used in query strings.'
+        );
+    }
+
+    public function isCandidate(Tokens $tokens): bool
+    {
+        return $tokens->isTokenKindFound(T_STRING);
+    }
+
+    public function isRisky(): bool
+    {
+        return true;
+    }
+
+    public function fix(\SplFileInfo $file, Tokens $tokens): void
+    {
+        foreach ($tokens as $index => $token) {
+            if ($token->isGivenKind(T_STRING) && $token->getContent() === 'http_build_query') {
+                if ($this->isFunctionCall($tokens, $index)) {
+                    continue;
+                }
+
+                $this->fixFunction($tokens, $index);
+            }
+        }
+    }
+
+    public function getName(): string
+    {
+        return self::class;
+    }
+
+    public function getPriority(): int
+    {
+        // Should be run after SingleQuoteFixer (priority 0) and ArraySyntaxFixer (priority 1)
+        return -1;
+    }
+
+    public function supports(\SplFileInfo $file): bool
+    {
+        return true;
+    }
+
+    private function fixFunction(Tokens $tokens, int $functionIndex): void
+    {
+        $openParenthesisIndex = $tokens->getNextTokenOfKind($functionIndex, ['(']);
+        if ($openParenthesisIndex === null) {
+            return;
+        }
+
+        $closeParenthesisIndex = $tokens->findBlockEnd(Tokens::BLOCK_TYPE_PARENTHESIS_BRACE, $openParenthesisIndex);
+
+        $argumentCount = (new ArgumentsAnalyzer())
+            ->countArguments($tokens, $openParenthesisIndex, $closeParenthesisIndex);
+
+        // When third argument is already present and it is null, override its value.
+        if ($argumentCount >= 3) {
+            $thirdArgumentType = $this->getThirdArgumentInfo($tokens, $openParenthesisIndex, $closeParenthesisIndex);
+            if ($thirdArgumentType === null) {
+                return;
+            }
+
+            if ($thirdArgumentType->getName() === 'null') {
+                $this->setArgumentValueToAmp($tokens, $thirdArgumentType->getStartIndex());
+            }
+
+            return;
+        }
+
+        $tokensToInsert = [];
+
+        // Add second argument if not defined
+        if ($argumentCount === 1) {
+            $tokensToInsert[] = new Token(',');
+            $tokensToInsert[] = new Token([T_WHITESPACE, ' ']);
+            $tokensToInsert[] = new Token([T_STRING, 'null']);
+        }
+
+        // Add third argument (arg separator): ", &"
+        if ($argumentCount < 3) {
+            $tokensToInsert[] = new Token(',');
+            $tokensToInsert[] = new Token([T_WHITESPACE, ' ']);
+            $tokensToInsert[] = new Token([T_STRING, "'&'"]);
+        }
+
+        if (!empty($tokensToInsert)) {
+            $beforeCloseParenthesisIndex = $tokens->getPrevNonWhitespace($closeParenthesisIndex);
+            $tokens->insertAt($beforeCloseParenthesisIndex + 1, $tokensToInsert);
+        }
+    }
+
+    /**
+     * Detect if this is most probably function call (and not function import or function definition).
+     */
+    private function isFunctionCall(Tokens $tokens, int $index): bool
+    {
+        $previousIndex = $tokens->getPrevMeaningfulToken($index);
+
+        return $previousIndex !== null
+            && ($tokens[$previousIndex]->isGivenKind(CT::T_FUNCTION_IMPORT)
+                || $tokens[$previousIndex]->isGivenKind(T_FUNCTION));
+    }
+
+    private function getThirdArgumentInfo(
+        Tokens $tokens,
+        int $openParenthesisIndex,
+        int $closeParenthesisIndex
+    ): ?TypeAnalysis {
+        $argumentsAnalyzer = new ArgumentsAnalyzer();
+
+        $arguments = $argumentsAnalyzer->getArguments($tokens, $openParenthesisIndex, $closeParenthesisIndex);
+        $argumentIndex = array_slice($arguments, 2, 1, true);
+        $argumentInfo = $argumentsAnalyzer->getArgumentInfo($tokens, key($argumentIndex), reset($argumentIndex));
+
+        return $argumentInfo->getTypeAnalysis();
+    }
+
+    private function setArgumentValueToAmp(Tokens $tokens, int $argumentStartIndex): void
+    {
+        $ampToken = new Token([T_STRING, "'&'"]);
+        $tokens->offsetSet($argumentStartIndex, $ampToken);
+    }
+}

tests/Fixer/Fixtures/Correct.txt

@@ -0,0 +1,21 @@
+<?php declare(strict_types=1);
+
+namespace Lmc\CodingStandard\Fixer\Fixtures;
+
+class Correct
+{
+    public function assembleQueryString(): void
+    {
+        $queryString1 = http_build_query(['foo' => 'bar'], null, '&');
+
+        $queryString1WithComment = http_build_query(['foo' => 'bar'], /* Comment, with commas, */ null ,  '&');
+
+        $queryString1WithObject = http_build_query((object) ['foo' => 'bar'], null, '&');
+
+        $queryString2 = http_build_query(['foo' => 'bar', 'baz' => 'bat'], null, '&');
+
+        $queryString3 = http_build_query(['foo' => 'bar'], null, '&amp;');
+
+        $queryString4 = http_build_query(['foo' => 'bar'], null, ';');
+    }
+}

tests/Fixer/Fixtures/Fixed.txt

@@ -0,0 +1,23 @@
+<?php declare(strict_types=1);
+
+function http_build_query($should, $not, $change): void
+{
+    // this should not be affected
+}
+
+function assembleQueryString(): void
+{
+    $queryString1 = http_build_query(['foo' => 'bar'], null, '&');
+
+    $queryString2 = http_build_query(['foo' => 'bar'], 333, '&');
+
+    $queryString3 = http_build_query(['foo' => 'bar'], 333, '&');
+
+    $queryString4 = http_build_query(['foo' => 'bar'], 333, '&', PHP_QUERY_RFC3986);
+
+    $queryString5 = http_build_query(['foo' => 'bar'], null, '&', PHP_QUERY_RFC3986);
+
+    $queryString6 = http_build_query(['foo' => 'bar', 'baz' => 'ban'], null, '&', PHP_QUERY_RFC3986);
+
+    $queryString6 = http_build_query((object) ['foo' => 'bar', 'baz' => 'ban'], null, '&', PHP_QUERY_RFC3986);
+}

tests/Fixer/Fixtures/Wrong.txt

@@ -0,0 +1,23 @@
+<?php declare(strict_types=1);
+
+function http_build_query($should, $not, $change): void
+{
+    // this should not be affected
+}
+
+function assembleQueryString(): void
+{
+    $queryString1 = http_build_query(['foo' => 'bar']);
+
+    $queryString2 = http_build_query(['foo' => 'bar'], 333);
+
+    $queryString3 = http_build_query(['foo' => 'bar'], 333, null);
+
+    $queryString4 = http_build_query(['foo' => 'bar'], 333, null, PHP_QUERY_RFC3986);
+
+    $queryString5 = http_build_query(['foo' => 'bar'], null, null, PHP_QUERY_RFC3986);
+
+    $queryString6 = http_build_query(['foo' => 'bar', 'baz' => 'ban'], null, null, PHP_QUERY_RFC3986);
+
+    $queryString6 = http_build_query((object) ['foo' => 'bar', 'baz' => 'ban'], null, null, PHP_QUERY_RFC3986);
+}

tests/Fixer/SpecifyArgSeparatorFixerTest.php

@@ -0,0 +1,38 @@
+<?php declare(strict_types=1);
+
+namespace Lmc\CodingStandard\Fixer;
+
+use PhpCsFixer\Tokenizer\Tokens;
+use PHPUnit\Framework\TestCase;
+
+class SpecifyArgSeparatorFixerTest extends TestCase
+{
+    /**
+     * @test
+     * @dataProvider provideFiles
+     */
+    public function shouldFixCode(string $inputFile, string $expectedOutputFile): void
+    {
+        $fixer = new SpecifyArgSeparatorFixer();
+        $fileInfo = new \SplFileInfo(__DIR__ . '/Fixtures/' . $inputFile);
+        $tokens = Tokens::fromCode(file_get_contents($fileInfo->getRealPath()));
+
+        $fixer->fix($fileInfo, $tokens);
+
+        $this->assertStringEqualsFile(
+            __DIR__ . '/Fixtures/' . $expectedOutputFile,
+            $tokens->generateCode()
+        );
+    }
+
+    /**
+     * @return array[]
+     */
+    public function provideFiles(): array
+    {
+        return [
+            'Correct file should not be changed' => ['Correct.txt', 'Correct.txt'],
+            'Wrong file should be fixed' => ['Wrong.txt', 'Fixed.txt'],
+        ];
+    }
+}