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 #434

Merged
merged 1 commit into from
Nov 11, 2021
Merged

Allow Symfony 6 #434

merged 1 commit into from
Nov 11, 2021

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Jul 18, 2021

No description provided.

@nicolas-grekas
Copy link
Member

We should wait a bit before allowing Symfony 6 because we're still breaking BC there (eg adding return types).

@greg0ire greg0ire marked this pull request as draft July 19, 2021 18:15
@greg0ire
Copy link
Member

greg0ire commented Jul 19, 2021

Converting to draft until the right moment.

@wouterj
Copy link

wouterj commented Sep 17, 2021

Fyi, all types have been added and work on this PR can resume :)

@greg0ire greg0ire marked this pull request as ready for review September 19, 2021 12:06
@greg0ire greg0ire closed this Sep 19, 2021
@greg0ire greg0ire reopened this Sep 19, 2021
@derrabus
Copy link
Member

derrabus commented Nov 8, 2021

Can you please rebase and resolve the conflicts?

@Kocal
Copy link
Contributor Author

Kocal commented Nov 8, 2021

Done, however I think it won't works because I can't set Composer's minimum-stability anymore.

Should we wait for Symfony 6 official release to re-run this PR's checks?

@derrabus
Copy link
Member

derrabus commented Nov 9, 2021

Done, however I think it won't works because I can't set Composer's minimum-stability anymore.

@greg0ire Can you have a look, please?

@greg0ire greg0ire force-pushed the symfony-6 branch 5 times, most recently from ed27345 to fe01803 Compare November 9, 2021 23:11
@greg0ire greg0ire marked this pull request as draft November 9, 2021 23:14
@greg0ire
Copy link
Member

greg0ire commented Nov 9, 2021

I've tried something, but it's getting late, and I have to think this through a bit more (the extra prefer-lowest job and upload coverage job seem wrong)

@greg0ire
Copy link
Member

I removed my commit because I cannot find a satisfying solution. I think in this case, we should partially revert 739d439, or just maintain an MR open that sets the stability in composer.json to dev, and merge it (after dropping the commit that does this) once Symfony 6 comes out. Having a workflow that oscillates between a custom version and a generic one does not sound great.

@derrabus
Copy link
Member

@greg0ire But adjusting the minimum stability for a CI job is not a very uncommon need. Wouldn't it be possible to add an optional parameter to the workflow that would result in a composer config minimum-stability $value being run before installing the dependencies? That's all we need here, I guess.

composer.json Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

But adjusting the minimum stability for a CI job is not a very uncommon need. Wouldn't it be possible to add an optional parameter to the workflow that would result in a composer config minimum-stability $value being run before installing the dependencies? That's all we need here, I guess.

We would also need an extra job under include in the build matrix, and currently there does not seem to be a straightforward way to make it conditional (I'm assuming we don't want to do this for every Doctrine repository, can be challenged at the org level).

@greg0ire greg0ire force-pushed the symfony-6 branch 2 times, most recently from 0fa2a98 to ec04d87 Compare November 11, 2021 09:45
composer.json Outdated Show resolved Hide resolved
@derrabus
Copy link
Member

We would also need an extra job under include in the build matrix, and currently there does not seem to be a straightforward way to make it conditional

I see.

Would it be possible to decouple the workflow from the matrix? My idea would be to have one parameterized phpunit workflow that runs the tests for a single given php-version and dependencies setting. The continuous-integration workflow run this new phpunit workflow with different parameters.

This way, we could add additional parameters to the phpunit workflow that the continuous-integration matrix does not make use of, like minimum-stablity. A project could use the continuous-integration workflow and add additional phpunit jobs with custom settings.

@derrabus
Copy link
Member

Psalm's ReservedWord check is broken. It complains because Symfony has added mixed type declarations to parameters. I'll prepare a bug report for Psalm, but for now I've disabled it because it reports false positives.

@derrabus derrabus marked this pull request as ready for review November 11, 2021 10:57
Co-authored-by: Grégoire Paris <[email protected]>
Co-authored-by: Alexander M. Turek <[email protected]>
Signed-off-by: Alexander M. Turek <[email protected]>
@derrabus derrabus merged commit 2359fbc into doctrine:3.1.x Nov 11, 2021
@derrabus derrabus modified the milestones: 3.1.3, 3.2.1 Nov 11, 2021
@greg0ire
Copy link
Member

greg0ire commented Nov 11, 2021

Would it be possible to decouple the workflow from the matrix? My idea would be to have one parameterized phpunit workflow that runs the tests for a single given php-version and dependencies setting. The continuous-integration workflow run this new phpunit workflow with different parameters.

This way, we could add additional parameters to the phpunit workflow that the continuous-integration matrix does not make use of, like minimum-stablity. A project could use the continuous-integration workflow and add additional phpunit jobs with custom settings.

Yes, that would be possible and was in fact quite similar to what I did with my previous attempt. There are things left to get right though, like how to upload coverage. I'll give it another try.

@Kocal Kocal deleted the symfony-6 branch November 11, 2021 13:01
@greg0ire
Copy link
Member

My new attempt can be seen at #456

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.

6 participants