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

Test against PHP 8, allow ECS 9 & upgrade config to PHP file #34

Merged
merged 1 commit into from Mar 19, 2021

Conversation

stefandoorn
Copy link
Contributor

No description provided.

@stefandoorn stefandoorn force-pushed the feat/upgrade-config-php8-ecs9 branch 3 times, most recently from 0eb84fd to 9398b2a Compare February 8, 2021 15:27
@stefandoorn
Copy link
Contributor Author

Took inspiration from #33, including the config file format change.

Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

It fixes the build and introduces new PHP and ECS version... I like it 😄 ping @pamil as you're the boss here

@@ -13,9 +13,25 @@ Installation & usage
$ composer require --dev sylius-labs/coding-standard
```

2. Include a configuration file in your `easy-coding-standard.yml`:
2. Import the configuration file in your `ecs.php`:
Copy link
Member

Choose a reason for hiding this comment

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

ecs.php -> easy-coding-standard.php? no need to change the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, but as we are changing the extension already, I figured we can follow the official ECS docs for the filename: https://github.com/symplify/easy-coding-standard#usage

->call('configure', [['use' => 'echo']]);

$services->set(ArraySyntaxFixer::class)
->call('configure', [['syntax' => 'short']]);
Copy link
Member

Choose a reason for hiding this comment

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

$services
    ->set(ArraySyntaxFixer::class)
    ->call('configure', [['syntax' => 'short']])
;

it looks much better in my opinion, but maybe it's only me 🖖

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 now applied ECS actually to ecs.php & also did that in the tests. So the config needs to be compatible with the standards itself, etc. etc. etc. :-D

@stefandoorn stefandoorn force-pushed the feat/upgrade-config-php8-ecs9 branch 2 times, most recently from 20835ae to c06da2a Compare February 19, 2021 13:58
@stefandoorn
Copy link
Contributor Author

cc @loevgaard took a bit of inspiration from #33 to make this PR

@loevgaard
Copy link
Contributor

Nice if we can make this work :D Thanks @stefandoorn 🎉

@stefandoorn
Copy link
Contributor Author

Ready to go @Zales0123 @pamil - WDYT?

composer.json Outdated
"slevomat/coding-standard": "^6.3",
"symplify/easy-coding-standard": "^7.3 || ^8.1"
"symplify/easy-coding-standard": "^7.3 || ^8.1 || ^9.0"
Copy link
Contributor

@pamil pamil Mar 4, 2021

Choose a reason for hiding this comment

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

What do you think about dropping ECS ^7.3 and ^8.1 and just supporting ^9.0? This PR would require a major release anyway, so we might cut down on supported packages.

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 saw many people running into an ECS error on Slack and I have it myself:

$ vendor/bin/ecs check src/ tests/
 [WARNING] You are using YAML format in "easy-coding-standard.yml" config.      
           It is deprecated and will be removed in next ECS 9. Switch to PHP    
           format as soon as possible with                                      
           "https://github.com/migrify/config-transformer"                      
 [ERROR] in_array() expects parameter 2 to be array, null given  

It seems not going to be fixed in the ECS version that we provide, so nobody will be able to get around this on older technology. This even happens on a plain project with only this requirement:

"sylius-labs/coding-standard": "^3.2"

But also on Sylius I've seen it happen on Slack.

If the ECS version is supporting anything from PHP 7.3, then I think we can be good only going with ^9.0. Users need to make a config file inclusion change etc. anyway, so it might be ok to do.

Let me know if you want me to adjust it.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we can just supporting Ecs ^9.0 now 👍🏻

@stefandoorn stefandoorn force-pushed the feat/upgrade-config-php8-ecs9 branch from 1365d2e to 19aab20 Compare March 4, 2021 19:04
@stefandoorn
Copy link
Contributor Author

I've adjusted the PR to be only ECS ^9.0 :-)

vvasiloi added a commit to vvasiloi/supplier-plugin that referenced this pull request Mar 10, 2021
@oallain
Copy link
Contributor

oallain commented Mar 19, 2021

Hello @pamil

What do you think about this latest version?

@pamil pamil merged commit cd4713f into SyliusLabs:master Mar 19, 2021
@pamil
Copy link
Contributor

pamil commented Mar 19, 2021

Thanks, Stefan! 🥇

@oallain oallain mentioned this pull request Mar 19, 2021
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

5 participants