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

Laravel components v8 #2576

Merged
merged 25 commits into from
Mar 5, 2021
Merged

Laravel components v8 #2576

merged 25 commits into from
Mar 5, 2021

Conversation

luceos
Copy link
Member

@luceos luceos commented Jan 27, 2021

**Fixes #2538 **

Still needs to be tested inside the skeleton.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

Companion PRs

@luceos luceos changed the title Dk/illuminate v8 Laravel components v8 Jan 27, 2021
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a 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.

composer.json Show resolved Hide resolved
@luceos
Copy link
Member Author

luceos commented Jan 27, 2021

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 luceos changed the title Laravel components v8 [WIP] Laravel components v8 Jan 27, 2021
@askvortsov1
Copy link
Sponsor Member

@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:

  • Extensions requiring fof/console will break, as that depends on illuminate/console v6, which depends on symfony console v4, which conflicts with core's use of symfony console v5.
  • The signature of setKeysForSaveQuery on line 65 of TagState in tags needs the typehint removed to comply with Laravel's interface / abstract class.

Other than that, I haven't found issues in the things I tried.

@luceos luceos changed the title [WIP] Laravel components v8 Laravel components v8 Mar 1, 2021
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a 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

composer.json Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@luceos luceos requested a review from askvortsov1 March 3, 2021 14:25
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a 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

Copy link
Contributor

@tankerkiller125 tankerkiller125 left a 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

@askvortsov1 askvortsov1 merged commit 84ded0c into master Mar 5, 2021
@askvortsov1 askvortsov1 deleted the dk/illuminate-v8 branch March 5, 2021 14:43
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.

Upgrade Illuminate components to 8.x
3 participants