-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Laravel components v8 #2576
Laravel components v8 #2576
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Laravel upgrade guide, a dependence on Symfony 5 is listed as one of the high impact changes, but we don't seem to have changed our symfony version at all: https://laravel.com/docs/7.x/upgrade#symfony-5-related-upgrades. Symfony 5 seems to be implicitly required for console though. Are there downsides to having separate versions of symfony for different symfony components?
It seems like the most impactful change would be the deprecation of the Translator interface that's used commonly in extensions.
I still need to go over testing inside the skeleton as mentioned in OP. I bet there are a ton of other issues to fix before this all works ;) |
@luceos I tested this with a skeleton install, and made the above 2 commits, which mitigate most issues with extensions that use the translator. Aside from that, there are a few issues we won't be able to compensate for:
Other than that, I haven't found issues in the things I tried. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one very minor formatting issue in composer.json
[ci skip] [skip ci]
This avoids an immediate BC break for classes in extensions that implement this interface.
[ci skip] [skip ci]
bc4b187
to
aeddce6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's a few more phpunit related changes needed to the tests before this can be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the test are good I'm happy with this
**Fixes #2538 **
Still needs to be tested inside the skeleton.
Confirmed
composer test
).Required changes:
Companion PRs