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

Add Symfony 5 compatibility #6263

Closed
wants to merge 2 commits into from

Conversation

franmomu
Copy link
Member

@franmomu franmomu commented Aug 9, 2020

Subject

Since #6212, KnpMenuBundle is the only external library that is not compatible with Symfony 5 on the stable branch.

So I used #5948 that modifies classes that are marked as final.

The other BC-break that I introduced (that I'm aware of) is to add a needed return type iterable to getExtendedTypes() method in the classes (marked as final also) that implement FormTypeExtensionInterface.

I copied ControllerTrait from Symfony since it's not available in Symfony 5.

I'm aware of #6093 and this is complementary since looks like the main problem for 4.0 is with CRUDController, so that still needs to be solved.

So the question is if this can be acceptable in the stable branch or not, if so we should also consider applying similar to #6212 (adding support to BlockBundle 4) to the bundles we want to be compatible with Symfony 5.

I am targeting this branch, because it would be nice to have Symfony 5 compatibility on the stable branch.

Closes #5788.

Changelog

### Added
- Added support for Symfony 5.

To do

  • Release SonataIntlBundle.
  • Fix PHPStan.
  • Update the documentation.
  • Add an upgrade note.

@franmomu franmomu added the minor label Aug 9, 2020
@franmomu franmomu changed the title Add Symfony 5 compabitility Add Symfony 5 compatibility Aug 9, 2020
@vv12131415
Copy link
Contributor

The other BC-break that I introduced (that I'm aware of) is to add a needed return type iterable to getExtendedTypes()

what you can do (which does not break BC, but looks ugly)
is something like this

if ($symfonyVersion === 4) {

class ChoiceTypeExtension extends AbstractTypeExtension
    {
        public static function getExtendedTypes()
        {
            return [ChoiceType::class];
        }
    }
} else {
class ChoiceTypeExtension extends AbstractTypeExtension
    {
        public static function getExtendedTypes(): iterable
        {
            return [ChoiceType::class];
        }
    }
}

this will be a workaround for the symfony 5. And later SonataAdmin 4.x will be released

@vv12131415
Copy link
Contributor

why don't you do the same if/else (ugly) thing with KnpMenuBundle?

I think if/else is more soft, than breaking things the hard way.

With it (if/else) I'll at least will not break my app if I don't extend classes with BC break.

@franmomu
Copy link
Member Author

why don't you do the same if/else (ugly) thing with KnpMenuBundle?

I think if/else is more soft, than breaking things the hard way.

With it (if/else) I'll at least will not break my app if I don't extend classes with BC break.

I think this case is different,

In this case we are not supporting 2.x version of KnpMenuBundle, just 3.x so

why don't you do the same if/else (ugly) thing with KnpMenuBundle?

I think if/else is more soft, than breaking things the hard way.

With it (if/else) I'll at least will not break my app if I don't extend classes with BC break.

I created #6275 where we can discuss about that and leave this to Symfony.

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@dmaicher
Copy link
Contributor

For one of my projects I also need Symfony 5 compat.

Is there any way I can help move this forward? Would be happy to.

@greg0ire
Copy link
Contributor

Thanks for the offer @dmaicher ! You can start by carrying #6275 if you want :)

@jaikdean
Copy link
Contributor

Looks like this is unblocked now. How can I help?

"symfony/dependency-injection": "^4.4.3",
"symfony/doctrine-bridge": "^4.4",
"symfony/event-dispatcher": "^4.4",
"symfony/asset": "^4.4 || ^5.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for ^4.4 || ^5.1 on Symfony packages rather than ^4.4 || ^5.0? Is there something hard to support in 5.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Symfony 5.0 is EOL (not supported anymore). See https://symfony.com/releases

So does not really make sense to support it here I believe 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

So it is, sorry 🤦

.travis.yml Outdated Show resolved Hide resolved
@wbloszyk
Copy link
Member

wbloszyk commented Sep 2, 2020

@franmomu
Could you move this PR to master branch? We can't add support for Symfony5 in Sonata3 becouse Symfony 5 - TODO list

@jaikdean
Copy link
Contributor

jaikdean commented Sep 2, 2020

@franmomu
Could you move this PR to master branch? We can't add support for Symfony5 in Sonata3 becouse Symfony 5 - TODO list

What BC breaks are in this PR? I've seen reference to BC breaks across a few issues/PRs, but not any specific examples.

public function buildView(FormView $view, FormInterface $form, array $options)
use Symfony\Component\Form\FormTypeExtensionInterface;

// NEXT_MAJOR: Remove the "else" part, copy all methods from BaseChoiceTypeExtension in this class and
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this is a no go for maintenance cost...

@wbloszyk
Copy link
Member

wbloszyk commented Sep 2, 2020

@jaikdean

        "symfony/framework-bundle": "^4.4 || ^5.1",

Using ^5.1 will not be possible - class CoreController extends Controller, but Symfony\Bundle\FrameworkBundle\Controller\Controller don't exists in Symfony5.

@SonataCI
Copy link
Collaborator

SonataCI commented Sep 2, 2020

Could you please rebase your PR and fix merge conflicts?

@jaikdean
Copy link
Contributor

jaikdean commented Sep 3, 2020

@jaikdean

        "symfony/framework-bundle": "^4.4 || ^5.1",

Using ^5.1 will not be possible - class CoreController extends Controller, but Symfony\Bundle\FrameworkBundle\Controller\Controller don't exists in Symfony5.

CoreController has been refactored in this PR to no longer extend Symfony\Bundle\FrameworkBundle\Controller\Controller.

@wbloszyk
Copy link
Member

wbloszyk commented Sep 4, 2020

@jaikdean

        "symfony/framework-bundle": "^4.4 || ^5.1",

Using ^5.1 will not be possible - class CoreController extends Controller, but Symfony\Bundle\FrameworkBundle\Controller\Controller don't exists in Symfony5.

CoreController has been refactored in this PR to no longer extend Symfony\Bundle\FrameworkBundle\Controller\Controller.

It is BC-break.

What about translator? There are huge BC-break too.
#6342

IMO better will be simply add support for Symfony5 in Sonata4, without BC-break. This will save time which we can spend on major release. This move will also encourage people to upgrade Admin to v4 and make less work for as to support v3.

@alytvynov
Copy link

Hello!
I suddenly found that sonata not compatible with symfony/console ^5.0.
Here in packaginst https://packagist.org/packages/sonata-project/admin-bundle sonata needs symfony/console: ^4.4. Anybody know something about this issue ?

@VincentLanglet
Copy link
Member

Hello!
I suddenly found that sonata not compatible with symfony/console ^5.0.
Here in packaginst https://packagist.org/packages/sonata-project/admin-bundle sonata needs symfony/console: ^4.4. Anybody know something about this issue ?

See #6365
Sonata 3.x won't be compatible with Symfony 5. We working on Sonata 4.

@alytvynov
Copy link

Thank you for your answer.

@VincentLanglet
Copy link
Member

Closing in favor of #6476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symfony 5 compatibility
10 participants