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

[rector] add more default sets, bump php cs fixer, phpunit #260

Open
wants to merge 2 commits into
base: 2.5
Choose a base branch
from

Conversation

TomasVotruba
Copy link
Contributor

No description provided.

SymfonySetList::SYMFONY_CONSTRUCTOR_INJECTION,
// activate when doing updates:
// SymfonyLevelSetList::UP_TO_SYMFONY_63,
// activate when doing updates:
// PHPUnitLevelSetList::UP_TO_PHPUNIT_90,
// PHPUnitSetList::PHPUNIT_91,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've bumped the PHPUnit to 10, which is the highest version with BC changes so far, so no need for extra upgrade sets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is strange is that a basic symfony still installs PHPUnit 9:

composer create-project symfony/skeleton
cd skeleton
composer require test

Need todo some research why it is so 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think flex + related hacks are maintained much. Composer require works well:

composer create-project symfony/skeleton
cd skeleton
composer require phpunit/phpunit

image

@TomasVotruba
Copy link
Contributor Author

Good to go 👍

@TomasVotruba
Copy link
Contributor Author

If I may suggest, phpunit-bridge is suitable for developing symfony/symfony core monorepo, as they need to test various conflicting PHP versions. Not for own tools/projects.
We use PHP 8.1+ here, so we're good with bare PHPUnit 👍 It also works better with PHPStan and Rector.

@TomasVotruba
Copy link
Contributor Author

Just an experiment to see if CI passes 👍

if (\is_file(\dirname(__DIR__) . '/vendor/phpunit/phpunit/phpunit')) {
\define('PHPUNIT_COMPOSER_INSTALL', \dirname(__DIR__) . '/vendor/autoload.php');
require PHPUNIT_COMPOSER_INSTALL;
PHPUnit\TextUI\Command::main();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class does not exist anymore, that's why CI was failing.
There is no added value of this custom bin file over vendor/bin/phpunit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will check this with the symfony recipe. because if we don't provide it the flex recipe will automatically create it.

@alexander-schranz
Copy link
Member

alexander-schranz commented Nov 19, 2024

I tried the config in a existing project and stumbled over an unexpected behaviour trying out the config.

The project using PHPUnit 9. Rector wants to update the attributes, think would nice to avoid that if PHPUnit 9 is used as I think attributes only supported by PHPUnit 10?:

rector.php
<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\Doctrine\Set\DoctrineSetList;
use Rector\PHPUnit\PHPUnit100\Rector\Class_\StaticDataProviderClassMethodRector;
use Rector\PHPUnit\Set\PHPUnitLevelSetList;
use Rector\PHPUnit\Set\PHPUnitSetList;
use Rector\Set\ValueObject\LevelSetList;
use Rector\Set\ValueObject\SetList;
use Rector\Symfony\CodeQuality\Rector\MethodCall\LiteralGetToRequestClassConstantRector;
use Rector\Symfony\Set\SymfonyLevelSetList;
use Rector\Symfony\Set\SymfonySetList;
use Sulu\Rector\Set\SuluLevelSetList;

return RectorConfig::configure()
    ->withPaths([
        __DIR__ . '/src',
        __DIR__ . '/tests',
    ])
    ->withRootFiles()
    ->withPhpSets()
    ->withPreparedSets(
        codeQuality: true,
        doctrineCodeQuality: true,
        symfonyCodeQuality: true,
        deadCode: true,
        codingStyle: true,
        instanceOf: true,
        typeDeclarations: true,
    )
    ->withImportNames(importShortClasses: false)
    ->withAttributesSets(all: true)
    ->withSymfonyContainerPhp(__DIR__ . '/var/cache/website/dev/App_KernelDevDebugContainer.xml')
    ->withSets([
        // - activate when doing symfony updates:
        // SymfonyLevelSetList::UP_TO_SYMFONY_64,
        // - activate when doing phpunit updates:
        // PHPUnitSetList::PHPUNIT_91,
        // PHPUnitSetList::PHPUNIT_100,
        // - activate when doing sulu updates:
        // SuluLevelSetList::UP_TO_SULU_25,
    ])
    ->withPHPStanConfigs([
        __DIR__ . '/phpstan.dist.neon',
        // rector does not load phpstan extension automatically so require them manually here:
        __DIR__ . '/vendor/phpstan/phpstan-doctrine/extension.neon',
        __DIR__ . '/vendor/phpstan/phpstan-symfony/extension.neon',
    ]);

Bildschirmfoto 2024-11-19 um 18 34 56

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Nov 19, 2024

The project using PHPUnit 9. Rector wants to update the attributes

I'll look into Rector attribute sets.

There composer.json here requires "phpunit/phpunit": "^10.5", so we're safe.

@alexander-schranz
Copy link
Member

I know but is there a way to use this config and still use PHPUnit 9 avoid the attributes upgrade only for PHPUnit? In some other cases were we use Rector we want to use same configs. Maybe we can fix it directly in Rector with check for if (!Composer::satisfies('phpunit/phpunit', '<10.0')) { not todo the upgrade, sure the one using phar wouldn't be catched but atleast the once who have phpunit in the composer.json and use still the older versions will no be updated automatically.

@alexander-schranz
Copy link
Member

The CI error looks unrelated to your changes. I will have a look at it.

PS: Like the codeQuality rules nice little catches in them 👍

@TomasVotruba
Copy link
Contributor Author

I know but is there a way to use this config and still use PHPUnit 9 avoid the attributes upgrade only for PHPUnit? In some other cases were we use Rector we want to use same configs.

Not yet, but I'll add it to Rector. Valid point 👍

@TomasVotruba
Copy link
Contributor Author

My work is done here, need to continue upgrading :)
Feel free to cherry pick what you like and drop the rest 👍

@alexander-schranz
Copy link
Member

@TomasVotruba thank you :)

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.

2 participants