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

chore: symfony 6 support #146

Merged
merged 3 commits into from
May 24, 2023
Merged

chore: symfony 6 support #146

merged 3 commits into from
May 24, 2023

Conversation

laurent-bientz
Copy link
Contributor

If there is no reason to block symfony 6

@greg0ire
Copy link
Owner

require-dev should also change. Also, Travis no longer seems to work… I've started a PR to make tests run on GA, but I've got a weird error. Anyway, if you use Symfony 6, then you must use PHP 8, and if you do, you should switch to native enums IMO.

@greg0ire
Copy link
Owner

Pipeline switched to Github Actions, please rebase 😎

@laurent-bientz
Copy link
Contributor Author

thanks @greg0ire and yes you're totally right, i'm a using PHP8 + Symfony 6 so I can use native Enum.

Long story short, i'm migrating a huge customer project from 5.4 LTS to 6.x (for future LTS) and before converting more than 180 Enum, custom FormTypes, normalizer and extensions, I prefer to iterate step by step ;)

chore: symfony 6 support
@greg0ire
Copy link
Owner

Fair enough I guess… the build is broken now.

@laurent-bientz
Copy link
Contributor Author

laurent-bientz commented May 22, 2023

unable to understand what's going wrong with the tests

the error seems to be related to Symfony himself in symfony\validator\Test\ConstraintValidatorTestCase.php with a ParseError: syntax error, unexpected variable "$constraints"

image

maybe a specific syntax error related to php 8.0 :/

tests are ok on php 8.1:

image

@greg0ire
Copy link
Owner

You can change the github workflow to 8.1 or 8.2 if you want to, I'm not super interested in checking that things work on 8.0, it's no longer a supported php version (apart from security fixes)

@laurent-bientz
Copy link
Contributor Author

seems to be a dead end because we need phpunit v9 to make it works but phpunit 9 is php 7.3+ and you still support PHP7.2

no worries, I can close, I'll work on my fork, I understand that my asking is a little weirdo.

thanks for your hard work on this helpful library that will be replaced by native PHP enum 😍

@greg0ire
Copy link
Owner

@laurent-bientz please reopen, it's not a dead end: you can require PHP 8 or 9, just like here: https://github.com/doctrine/orm/blob/2.15.x/composer.json#L46

@laurent-bientz
Copy link
Contributor Author

laurent-bientz commented May 23, 2023

I will retry later, the fact is not to use the correct version of phpunit, a part of your tests suite should use phpunit 8 if php 7.2 and another part should use phpunit 9 if php 8.1, so I don't know how to deal with that depending on the matrix in the CI.

actually the version of phpunit is fixed here

your actual tests are ok with phpunit 8.5.13 if PHP < 8 but phpunit 8.5.13 fails with PHP8+

the tests are ok with phpunit 9 and PHP8+ but fails on php 7.2

so I'm a little stuck :/

@greg0ire
Copy link
Owner

One way would be to use if, like here: https://github.com/doctrine/.github/blob/main/.github/workflows/continuous-integration.yml#L46

You define one block per phpunit version, and trigger each block conditionally based on the php version.

@laurent-bientz
Copy link
Contributor Author

looks good @greg0ire, thanks a lot for your help !

@@ -51,8 +51,16 @@ jobs:
dependency-versions: "${{ matrix.dependencies }}"
composer-options: "${{ inputs.composer-options }}"

- name: "Run PHPUnit"
- name: "Run PHPUnit for PHP 7x"
if: "${{ matrix.php-version != '8.0' }}"
Copy link
Owner

Choose a reason for hiding this comment

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

This seems more future proof:

Suggested change
if: "${{ matrix.php-version != '8.0' }}"
if: "${{ startsWith(matrix.php-version, '7') }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed!

run: >
SYMFONY_DEPRECATIONS_HELPER="max[self]=0"
SYMFONY_PHPUNIT_VERSION=8.5.13
vendor/bin/simple-phpunit

- name: "Run PHPUnit for PHP 8.0"
if: "${{ matrix.php-version == '8.0' }}"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if: "${{ matrix.php-version == '8.0' }}"
if: "${{ ! startsWith(matrix.php-version, '7') }}"

chore(ci): use a different phpunit version depending on php version
@greg0ire
Copy link
Owner

Thanks @laurent-bientz !

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