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.

Sign up for GitHub

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

Jump to bottom

Add support for Symfony 5 / admin 4 #641

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Add support for Symfony 5 / admin 4 #641

merged 1 commit into from
Jan 10, 2022

Conversation

jorrit
Copy link
Contributor

@jorrit jorrit commented Jan 6, 2022

Subject

Add support for Symfony 5, Sonata Admin 4, Twig 3.

Closes #463.

Changelog

### Added
- Added support for Symfony 5 / Sonata Admin 4 / Twig 3.

To do

  • Make sure everything works.

@jorrit jorrit force-pushed the symfony5 branch 2 times, most recently from bb8c255 to 7e0ea33 Compare January 6, 2022 14:03
jordisala1991
jordisala1991 requested changes Jan 7, 2022
View reviewed changes
@jorrit
Copy link
Contributor Author

jorrit commented Jan 7, 2022

Thanks for your review, @jordisala1991 !

@jordisala1991
Copy link
Member

Can you edit the workflow file?

You need to add symfony 5.3 and 5.4. It will need also a dev-kit PR, but we would like to see if all the build pass.

It would also be nice if you try to add symfony 6.0 to the composer.json too (and on the workflow file)

@jorrit
Copy link
Contributor Author

jorrit commented Jan 7, 2022 •
edited
Loading

Is the build variant for sonata-project/block-bundle:"3.*" still relevant? If not, I think I need to create a PR on devkit.

@jorrit
Copy link
Contributor Author

jorrit commented Jan 7, 2022

friendsofsymfony/ckeditor-bundle is not ready for Symfony 6.

@VincentLanglet
Copy link
Member

Is the build variant for sonata-project/block-bundle:"3.*" still relevant? If not, I think I need to create a PR on devkit.

It's not relevant, we cannot support both sonata admin 3 and 4. And Sonata admin 4 only use block 4.

@jorrit jorrit marked this pull request as ready for review January 7, 2022 12:15
jordisala1991
jordisala1991 reviewed Jan 7, 2022
View reviewed changes
@VincentLanglet
Copy link
Member

There will be FriendsOfSymfony/FOSCKEditorBundle#239 to follow

@VincentLanglet
Copy link
Member

There will be FriendsOfSymfony/FOSCKEditorBundle#239 to follow

Merged, but their looking for a maintainer...

@jorrit
Copy link
Contributor Author

jorrit commented Jan 10, 2022

Perhaps we should not wait for a new release for the CKEditor bundle and add support for Symfony 5 now, and Symfony 6 when CKEditor is updated.

@jordisala1991
Copy link
Member

Perhaps we should not wait for a new release for the CKEditor bundle and add support for Symfony 5 now, and Symfony 6 when CKEditor is updated.

Can you open a dev-kit PR with the needed changes too?

@jorrit jorrit mentioned this pull request Jan 10, 2022
Merged
@jorrit
Copy link
Contributor Author

jorrit commented Jan 10, 2022

The dev-kit PR has been created.

VincentLanglet
VincentLanglet requested changes Jan 10, 2022
View reviewed changes
tests/Block/FormatterBlockServiceTest.php Outdated
@@ -48,7 +48,7 @@ public function testExecute(): void
/**
* @group legacy
*/
public function testExecuteWithDeprecatedTeamplating(): void
public function testExecuteWithDeprecatedTemplating(): void
{
if (!class_exists('Sonata\BlockBundle\Block\Service\AbstractAdminBlockService')) {
Copy link
Member

Choose a reason for hiding this comment

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

This test can be removed if we remove the support of block bundle 3

@jorrit
Copy link
Contributor Author

jorrit commented Jan 10, 2022

Done. I see two more tests marked @group legacy. When I remove those annotations, no deprecations show up in the test output. Can/should those annotations be removed in that case?

@VincentLanglet
Copy link
Member

Done. I see two more tests marked @group legacy. When I remove those annotations, no deprecations show up in the test output. Can/should those annotations be removed in that case?

Seems like there is more tests with group legacy https://github.com/sonata-project/SonataFormatterBundle/search?q=legacy ; at least on the default branch. I don't have the project locally to check if there is more.
Some of them have NEXT_MAJOR comment to help knowing what to do.

For tests without next major comment, the question would be to know why there is a group legacy. If you know why and it's not needed anymore you can remove the group. Anyway it can still be done in another PR. The stable release will be done after

  • Adding param/return type everywhere
  • Bumping Phpstan/psalm to good enough levels.
  • Solving all the next major comment
  • Mark as final as possible classes

@jorrit
Copy link
Contributor Author

jorrit commented Jan 10, 2022

I'm working on the 5.x branch and all the NEXT_MAJOR comments have been addressed. This leaves 2 legacy groups:

  • FormatterValidatorTest::testInvalidCase: Can't find why this one is legacy.
  • SonataFormatterExtensionTest::testLoadWithoutDefaultFormatter: I don't think this test case makes sense anymore because the deprecation it tested was marked as NEXT_MAJOR: remove this if block and removed in 4.x.

@VincentLanglet VincentLanglet merged commit 25ccae3 into sonata-project:5.x Jan 10, 2022
@VincentLanglet
Copy link
Member

I'm working on the 5.x branch and all the NEXT_MAJOR comments have been addressed. This leaves 2 legacy groups:

  • FormatterValidatorTest::testInvalidCase: Can't find why this one is legacy.
  • SonataFormatterExtensionTest::testLoadWithoutDefaultFormatter: I don't think this test case makes sense anymore because the deprecation it tested was marked as NEXT_MAJOR: remove this if block and removed in 4.x.

I've merge the dev-kit PR and this one but a new PR to remove the legacy group is welcomed.

Also, if you wanna work on the stable release of 5.0, it's welcomed too. We (@jordisala1991 and I) can help you about the TODO-list.

@jorrit jorrit mentioned this pull request Jan 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordisala1991 jordisala1991 jordisala1991 approved these changes

@VincentLanglet VincentLanglet VincentLanglet approved these changes

Assignees
No one assigned
Labels
major
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jorrit @jordisala1991 @VincentLanglet