Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Symfony 6 #6095

Merged
merged 1 commit into from
Dec 6, 2021
Merged

Conversation

derrabus
Copy link
Contributor

@derrabus derrabus commented Nov 5, 2021

Replaces #5966

This PR allows to install PHP-CS-Fixer in Symfony 6 applications.

@coveralls
Copy link

coveralls commented Nov 5, 2021

Coverage Status

Coverage remained the same at 92.847% when pulling 2e57149 on derrabus:improvement/symfony-6 into f16bb74 on FriendsOfPHP:master.

@derrabus
Copy link
Contributor Author

derrabus commented Nov 5, 2021

Seems like Prophecy is having some problems here: phpspec/prophecy#527

@derrabus derrabus force-pushed the improvement/symfony-6 branch 3 times, most recently from b6de401 to b5c4547 Compare November 6, 2021 06:17
@derrabus derrabus mentioned this pull request Nov 8, 2021
tests/Linter/ProcessLintingResultTest.php Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mamchyts
Copy link

any updates?

@TomasVotruba
Copy link

TomasVotruba commented Nov 30, 2021

Just FYI, this is last blocker for Symplify and Rector ecosystem to upgrade to Symfony 6. Any ETA on moving this forward?

@TomasVotruba
Copy link

TomasVotruba commented Nov 30, 2021

Temporary solution to use this PR for those whom need it:

{
    "require-dev": {
        "friendsofphp/php-cs-fixer": "dev-improvement/symfony-6 as 3.3.2"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/derrabus/PHP-CS-Fixer"
        }
    ]
}

@julienfalque
Copy link
Member

Do Simplify and Rector actually need to be installed along with other app's dependencies? If not, I'd suggest using a different installation method, e.g. a dedicated composer.json, which would prevent dependencies conflicts.

@TomasVotruba
Copy link

TomasVotruba commented Dec 1, 2021

@julienfalque Unfortunately yes, Symplify ECS uses php-cs-fixer directly as it wraps it, and Rector needs coding standard tool to tidy up spaces.

@soxyl
Copy link

soxyl commented Dec 1, 2021

I get the point that it would be best to install php-cs-fixer separately from the actual project dependencies. Yet, using GrumPHP along with its php-cs-fixer task requires me to install php-cs-fixer as a dev dependency of the main project.

Therefore it would be nice to see support for Symfony 6 soon. 😃

@derrabus
Copy link
Contributor Author

derrabus commented Dec 1, 2021

CI fails because of php-coveralls/php-coveralls#327 now.

@ruudk
Copy link
Contributor

ruudk commented Dec 1, 2021

Why is that a dependency for php-cs-fixer ;)?

@SpacePossum
Copy link
Contributor

SpacePossum commented Dec 1, 2021

The test is not about the process of linting itself but how the result of a Process that does such linting is transformed into a result object. I wander if updating the test into the following would suffice as replacement:

<?php

declare(strict_types=1);

/*
 * This file is part of PHP CS Fixer.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *     Dariusz Rumiński <dariusz.ruminski@gmail.com>
 *
 * This source file is subject to the MIT license that is bundled
 * with this source code in the file LICENSE.
 */

namespace PhpCsFixer\Tests\Linter;

use PhpCsFixer\Linter\LintingException;
use PhpCsFixer\Linter\ProcessLintingResult;
use PhpCsFixer\Tests\TestCase;
use Symfony\Component\Process\Process;

/**
 * @internal
 *
 * @covers \PhpCsFixer\Linter\ProcessLintingResult
 */
final class ProcessLintingResultTest extends TestCase
{
    public function testCheckOK(): void
    {
        $process = new Process(['ls']);
        $process->run();
        static::assertTrue($process->isSuccessful(), 'Test setup failure.');

        $result = new ProcessLintingResult($process);
        $result->check();
    }

    public function testCheckFail(): void
    {
        $process = new Process(['foo']);
        $process->run();
        static::assertFalse($process->isSuccessful(), 'Test setup failure.');

        $result = new ProcessLintingResult($process);

        $this->expectException(LintingException::class);
        $this->expectExceptionCode($process->getExitCode());

        $result->check();
    }
}

@keradus keradus added the RTM Ready To Merge label Dec 6, 2021
@SpacePossum
Copy link
Contributor

Thank you @derrabus.

@SpacePossum SpacePossum merged commit 4ab8dc5 into PHP-CS-Fixer:master Dec 6, 2021
@keradus
Copy link
Member

keradus commented Dec 6, 2021

extra note: during the recurring call, we discussed few approaches how to handle Symfony 6 while fix on Prophecy is not there yet.
We believe that none of suggested solution is ideal, yet we still wanted to move forward with the topic, thus we decided to merge.

@keradus keradus removed the RTM Ready To Merge label Dec 6, 2021
@TomasVotruba
Copy link

Thank you 👍

SpacePossum added a commit that referenced this pull request Dec 9, 2021
This PR was merged into the master branch.

Discussion
----------

DX: drop hack for Prophecy incompatibility

https://github.com/phpspec/prophecy/releases/tag/v1.15.0

follows up #6095

Commits
-------

6cb274a DX: drop hack for Prophecy incompatibility
@derrabus derrabus deleted the improvement/symfony-6 branch October 21, 2023 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants