Add support for Symfony 5#52
Conversation
|
Fixes #51 |
|
Thanks for the PR! Should we update this to work with Symfony 5 instead? It's meant to run as a standalone thing, so there's no conflict with whatever Drupal is using. |
|
@joachim-n Maybe, my original thought is that this project would be consistent with the version of Symfony that Drupal supports. Currently Drupal 9 supports 4.x, looks like there is a desire to make it work with 5.x but we aren't there yet: https://www.drupal.org/project/drupal/issues/3055180 |
|
@joachim-n Testing some changes to make it compatible anyway. |
|
Looks like some more changes are necessary to the other commands that are missing return statements. Example is the apply command, looking into it. |
|
It looks like we just need to return a 0 status code from commands? If Symfony < 5 doesn't mind this, then it seems like an easy win. You might want to make a different PR for this new approach though, rather than have the commits that change the composer.json kicking around. Thanks for looking into this! |
|
Yeah, should be an easy win and won't break anything on less than Symfony 5. I will squash the commits once everything appears to be working. |
|
@joachim-n Ok, I squashed the commits and force pushed the changes to the branch on my fork so everything is cleaned up now. I will say that of the commands I was able to test manually with the project I am working on everything appears to be functional. I would still recommend a power user checks these before merging:
When someone has a chance to test this I look forward to your feedback. |
|
This looks good to me, we had to do the same thing in core, because Symfony 4 started throwing deprecation errors if commands didn't return an int. I had considered creating a class constant to represent COMMAND_SUCCESS or something, but we return 0 in core and symfony returns 0, so I just stick with 0. |
Purpose:
Fixes:
Solution: