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

Symfony 5 #997

Merged
merged 5 commits into from
Dec 18, 2019
Merged

Symfony 5 #997

merged 5 commits into from
Dec 18, 2019

Conversation

kuraobi
Copy link
Contributor

@kuraobi kuraobi commented Dec 12, 2019

Updates #987.

So, I spent a bit of time on this Symfony 5 compatibility, and there was quite a lot to do actually:

  • I had to upgrade PHPStan to 0.12 and PHPUnit to 7.5 so that it can work. Upgrading PHPUnit forced me to fix some deprecations like using non namespaced classes or setExpectedException.
  • I got rid of the full dependency to https://github.com/voryx/Thruway, requiring only https://github.com/thruway/client instead. The client is enough for this lib, voryx/thruway is actually only needed for dev and testing, so I moved it to the thruway docker container.
  • I had to create alternate versions of the classes directly extending or type-hinting the EventDispatcher and the Event class from Symfony. Symfony completely changed those between 4.3 and 5.0. They provide a proxy to help with migrating from 4.3+ to 5.0, but the proxy is not enough and does not exist for prior versions. So it’s a bit messy but basically, the old Event class is supported for Symfony < 4.3, the new Event class is supported for Symfony >= 5.0, and the versions in between support both classes, allowing people to migrate their code.
  • Commands return values were sometimes null, which is not allowed anymore.
  • replaced deprecated kernel.root_dir with kernel.project_dir.
  • removed framework.templating option in config.yml, as it does not exist anymore.
  • adapted the MessageQueueCollector, since the DataCollector interface has changed.
  • adapted the RunCommandProcessor to use Process construction from array instead of string, which is deprecated.

@kuraobi kuraobi mentioned this pull request Dec 12, 2019
@makasim
Copy link
Member

makasim commented Dec 12, 2019

Thanks for your work @kuraobi.

We can drop support for Symfony prior to 4.3. You can do some BC breaks if it is appropriate. It is going to be released as 0.10.0 anyway so that's fine.

Hope it makes things easier.

@kuraobi kuraobi force-pushed the symfony-5 branch 2 times, most recently from fcd704a to d75eed7 Compare December 12, 2019 08:41
@Steveb-p
Copy link
Contributor

We can drop support for Symfony prior to 4.3. You can do some BC breaks if it is appropriate. It is going to be released as 0.10.0 anyway so that's fine.

FWIW Symfony 3.4 is still considered actively maintained for almost a year (https://symfony.com/releases). If compatibility can be maintained without major effort it would be a ➕ .

I don't particularly need 3.4 compatibility as I try to keep my own projects updated, but for some people it can be important 😞

@makasim
Copy link
Member

makasim commented Dec 12, 2019

@Steveb-p Symfony 3.x users can still use Enqueue 0.9 which is quite mature.

@kuraobi kuraobi force-pushed the symfony-5 branch 2 times, most recently from ec19d88 to 0a5dfbf Compare December 12, 2019 10:21
@kuraobi
Copy link
Contributor Author

kuraobi commented Dec 12, 2019

I fixed the build and it's all green on my fork (https://travis-ci.org/kuraobi/enqueue-dev/builds/624082381), but the build fails on the PR due to some cache problem. It complains about a test class that don't even exist 😰

@kuraobi
Copy link
Contributor Author

kuraobi commented Dec 12, 2019

I'll drop support for < 4.3 in a separate branch, so that you can decide if you want to drop it or not.

@Steveb-p
Copy link
Contributor

I'll drop support for < 4.3 in a separate branch, so that you can decide if you want to drop it or not.

Feel free to drop it immediately. I agree with @makasim.

@kuraobi kuraobi force-pushed the symfony-5 branch 3 times, most recently from 3b00532 to a538233 Compare December 12, 2019 12:10
@makasim
Copy link
Member

makasim commented Dec 12, 2019

I'll drop support for < 4.3 in a separate branch, so that you can decide if you want to drop it or not.

Yeah, just drop it.

@kuraobi
Copy link
Contributor Author

kuraobi commented Dec 12, 2019

OK so I dropped 4.3 and fixed all the linter errors, you just need to clear the travis cache to get rid of the non existing DoctrineConnectionFactoryFactoryTest, then all the tests should be green.

@makasim
Copy link
Member

makasim commented Dec 12, 2019

OK so I dropped 4.3 and fixed all the linter errors, you just need to clear the travis cache to get rid of the non existing DoctrineConnectionFactoryFactoryTest, then all the tests should be green.

I cleared the cache and re-run the build. The error is still there. Have you tried to run tests locally? Does it work?

@kuraobi
Copy link
Contributor Author

kuraobi commented Dec 12, 2019

OK so I dropped 4.3 and fixed all the linter errors, you just need to clear the travis cache to get rid of the non existing DoctrineConnectionFactoryFactoryTest, then all the tests should be green.

I cleared the cache and re-run the build. The error is still there. Have you tried to run tests locally? Does it work?

That's weird, the tests work fine locally, as well as on Travis on my fork (https://travis-ci.org/kuraobi/enqueue-dev). The file where the error is supposed to be does not exist in the project.

@makasim
Copy link
Member

makasim commented Dec 13, 2019

Okay. I'll merge it as long as tests pass for your fork on Travis.

@makasim makasim merged commit 0bbba64 into php-enqueue:master Dec 18, 2019
@makasim
Copy link
Member

makasim commented Dec 19, 2019

Tagged 0.1.0.0.

@lsmith77
Copy link

@makasim there are several symfony dependencies that do not yet allow symfony 5.0 .. was this somehow intentional or just an oversight?

this is blocking liip/LiipImagineBundle#1246

@Steveb-p
Copy link
Contributor

@makasim there are several symfony dependencies that do not yet allow symfony 5.0 .. was this somehow intentional or just an oversight?

I'm almost sure it was an oversight on our part. What dependencies specifically are causing issues if you don't mind?

@makasim
Copy link
Member

makasim commented Dec 23, 2019

https://github.com/php-enqueue/enqueue-dev/blob/master/composer.json#L47-L56

@lsmith77 these particular dependencies should not be an issue since they are the deps of enqueue-dev. It is a development repo and is not listed on packagist at all.

Any way the deps linked have to be fixed. It's an error.

Are you requiring 0.10.0 version ?

@Steveb-p
Copy link
Contributor

Huh, seems like packages were updated with Symfony 5.0 dependency in their composer.json files, but the "main" repository file was not adjusted. I'll add a PR for it in the evening if no one else does in the meantime. There are some errors reported in CI that we'll have to look into as well.

Are you requiring 0.10.0 version ?

LiipBundle seems to have a dev dependency on 0.9:
https://github.com/liip/LiipImagineBundle/blob/c936c278f3fefd5f16da39f912c23237c546626b/composer.json#L38

They should be good to go once they switch it over to 0.10.

@lsmith77
Copy link

right of course .. and I didn't read the error messages properly on travis .. the issue is with enqueue/enqueue-bundle and we just need to allow ^0.10 there as well

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.

5 participants