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

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Allow Symfony 6 #6663

merged 1 commit into from
Nov 5, 2021

Conversation

loic425
Copy link
Contributor

@loic425 loic425 commented Oct 13, 2021

No description provided.

@loic425
Copy link
Contributor Author

loic425 commented Oct 13, 2021

@orklah I didn't see any configuration on your CI to test versions of Symfony.

@weirdan weirdan changed the title Add support for Symfony 6 Allow Symfony 6 Oct 14, 2021
@weirdan
Copy link
Collaborator

weirdan commented Oct 14, 2021

This is no-op until brianum/paratest allows newer symfony components:
image

@orklah
Copy link
Collaborator

orklah commented Oct 14, 2021

unless I'm mistaken, there is no release made for 6.0 yet neither
https://github.com/symfony/console/releases
https://github.com/symfony/process/releases

And we can't allow dev branches.

This will have to wait november

@loic425
Copy link
Contributor Author

loic425 commented Oct 14, 2021

@orklah Yes but Symfony encourages to be prepared as soon as possible and helping each third party packages to work on this before the release.

Moreover it will be released in a few weeks.

Some packages with Symfony 6 merged PR on main branch:

@loic425
Copy link
Contributor Author

loic425 commented Oct 14, 2021

@orklah Making this available on main branch allow third party packages to work on Symfony 6 support using your "dev-master".

@orklah
Copy link
Collaborator

orklah commented Oct 14, 2021

I'm all for preparing and checking if it's okay. If possible, I'd like to have a CI running somewhere with symfony 6 just to check that we are ready but there are a few constraints:

  • I don't know the release process of Symfony. Do they still commit on this branch? Is it possible they'll introduce a bug, a new bc break or a regression before release?
  • Symfony/console:6.0 doesn't exists yet as a stable version. This is what happens when we try to require it: "Could not find package symfony/console in a version matching "6.0" and a stability matching "dev".", so adding it as-is won't have any effect
  • We have dependencies ourselves that refuse symfony/console 6, those will have to be resolved first

So, if I summarize all this:
if we want to prepare for symfony 6, we have to have a CI job somewhere or a local execution that:

  • allows devs packages
  • don't have dependencies issues
  • that will allow to really install symfony 6 so we can use our test suite against

If this outputs new errors, those will have to be fixed. If not, it means it means we'll be ready to add ^6.0 to our accepted versions when symfony will release their package.

If we don't do that, it means we'll release a version now where we accept a package that we didn't test again and that may still change until the release. This means, the day where symfony release the package and our own dependencies allows it, we'll possibly break builds using the release we made without the fixes needed.

So, if you want to help, what you need to do change your composer.json locally and add https://getcomposer.org/doc/04-schema.md#minimum-stability to "dev", https://getcomposer.org/doc/04-schema.md#prefer-stable to true, and try require symfony 6. Ideally, it would pass and really install symfony 6, unfortunately, our dependancies will block you, so you need to propose a PR to them too so they'll allow it. Either they'll accept the PR, or you need to target your PR branch from your local psalm's composer file.

Once you're able to really install symfony 6 through composer, you have to run our test suite to see if anything breaks. If it doesn't, great! Push your composer.json in this PR and once we can remove minimum-stability, prefer-stable from it, it means we're ready to merge

@orklah
Copy link
Collaborator

orklah commented Oct 14, 2021

About behat, you'll note that's what they did here:
https://github.com/Behat/Behat/pull/1346/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R30

They introduced symfony 6 in their CI as a dev package to test against before merging.

@orklah
Copy link
Collaborator

orklah commented Oct 14, 2021

@orklah Making this available on main branch allow third party packages to work on Symfony 6 support using your "dev-master".

If you have trouble installing Psalm along with other packages, you should use https://github.com/psalm/phar instead, it has no dependencies so it won't bother you with that

Copy link
Collaborator

@orklah orklah left a comment

Choose a reason for hiding this comment

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

as we discussed, we need to have a full run of the test suite really using symfony 6 before we can merge

@loic425
Copy link
Contributor Author

loic425 commented Oct 14, 2021

@orklah Yes, I agree with you. I will work on this soon.

@orklah orklah marked this pull request as draft October 18, 2021 20:13
@loic425
Copy link
Contributor Author

loic425 commented Nov 5, 2021

@orklah I opened a PR on Paratest.

@loic425
Copy link
Contributor Author

loic425 commented Nov 5, 2021

@orklah 😵‍💫
https://github.com/paratestphp/paratest/runs/4116716725?check_suite_focus=true

What's next? Do you advice Paratest to use the psalm phar instead?

@weirdan
Copy link
Collaborator

weirdan commented Nov 5, 2021

I didn't realize we had circular dependencies.

@weirdan
Copy link
Collaborator

weirdan commented Nov 5, 2021

I'd be ok with merging and releasing just range extension, without CI changes. Worst case scenario is we have psalm-plugin not working in some combination of Psalm and Symfony versions, and that's no big deal in my book.

I don't see a need to test compatibility with every Symfony version on every push.

@loic425
Copy link
Contributor Author

loic425 commented Nov 5, 2021

I'd be ok with merging and releasing just range extension, without CI changes. Worst case scenario is we have psalm-plugin not working in some combination of Psalm and Symfony versions, and that's no big deal in my book.

I don't see a need to test compatibility with every Symfony version on every push.

I reverted the CI changes.

@weirdan weirdan added the release:fix The PR will be included in 'Fixes' section of the release notes label Nov 5, 2021
@weirdan weirdan marked this pull request as ready for review November 5, 2021 15:21
@weirdan weirdan merged commit 9651daf into vimeo:master Nov 5, 2021
@loic425 loic425 deleted the symfony-6-support branch November 5, 2021 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants