search

sonata-project / SonataPageBundle

This bundle provides a Site and Page management through container and block services
https://docs.sonata-project.org/projects/SonataPageBundle
MIT License
219 stars 210 forks source link

Remove deprecated code #1417

Closed eerison closed 2 years ago

eerison commented 2 years ago

⚠️ Deprecations that need to be solved for Symfony 5 upgrade.

Note those deprecations are waiting for this PR https://github.com/sonata-project/SonataAdminBundle/pull/7867

Remaining direct deprecation notices (6)

Deprecations related with bundles.

Warning if you going to work in one of those deprecations, say which one you will get before start, to avoid more the one guy work in the same thing!

Hanmac commented 2 years ago

"Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent::getException()" method is deprecated since Symfony 4.4, use "getThrowable()"

is getting fixed by this: #1429

Hanmac commented 2 years ago

The "Sonata\PageBundle\Twig\Extension\PageExtension" class implements "Twig\Extension\InitRuntimeInterface" that is deprecated since Twig 2.7, to be removed in 3.0.

isn't fixed by #1423 but will be by #1436

eerison commented 2 years ago

The "Sonata\PageBundle\Twig\Extension\PageExtension" class implements "Twig\Extension\InitRuntimeInterface" that is deprecated since Twig 2.7, to be removed in 3.0.

isn't fixed by #1423 but will be by #1436

Ok I changed ;) Thanks :)

Hanmac commented 2 years ago

2x: Passing a null message when instantiating a "Symfony\Component\Validator\ConstraintViolation" is deprecated since Symfony 4.4. 1x in UniqueUrlValidatorTest::testValidateWithPageFound from Sonata\PageBundle\Tests\Validator 1x in UniqueUrlValidatorTest::testValidateWithRootUrlAndNoParent from Sonata\PageBundle\Tests\Validator

will be fixed by this: https://github.com/sonata-project/SonataPageBundle/pull/1437

The Translator was the culprit

Hanmac commented 2 years ago

@VincentLanglet how do i fix these two?

1x: The "Sonata\AdminBundle\Admin\AbstractAdmin::getBaseRoutePattern()" method is considered final since sonata-project/admin-bundle 3.102. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Admin\SharedBlockAdmin". 1x in CreateSnapshotsCommandTest::setUp from Sonata\PageBundle\Tests\Command 1x: The "Sonata\AdminBundle\Admin\AbstractAdmin::getBaseRouteName()" method is considered final since sonata-project/admin-bundle 3.102. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Admin\SharedBlockAdmin".

caused by this: https://github.com/sonata-project/SonataPageBundle/blob/f66917525405ec7ecce0880043089bcf851019a1/src/Admin/SharedBlockAdmin.php#L39-L47

because these are "would be" final, i can't overwrite them like that, and if i would try to set them later, i can't overwrite the cached values?

VincentLanglet commented 2 years ago

You can override directly the protected variable https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Admin/AbstractAdmin.php#L112-L119

Can't it do the job ?

Hanmac commented 2 years ago

You can override directly the protected variable https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Admin/AbstractAdmin.php#L112-L119

Can't it do the job ?

Yes I could set these but that doesn't work like the current function

Because BaseBlockAdmin doesn't set these values they need to be calculated from class. And when they do, the cache values are set. Now setting the variable doesn't change anything because of the cache ones.

for example i tried this:


    public function configure()
    {
        $this->baseRoutePattern = sprintf('%s/%s', $this->getBaseRoutePattern(), 'shared');
        $this->baseRouteName = sprintf('%s/%s', $this->getBaseRouteName(), 'shared');
    }

which doesn't work because of cached values

VincentLanglet commented 2 years ago

Ok, I have some questions

It will help me to find the best solutions.

Hanmac commented 2 years ago

Ok, I have some questions

* What are the current values of routePattern and routeName with the override ? I see it's `%s/shared` but I need to know the whole value to understand why we can't reproduce it with
protected $baseRoutePattern = 'foo/shared`
protected $baseRouteName = 'foo/shared`
* Why does it matter to suffix this with `shared` ? Can't we just remove the override and let the default behavior ? This is weird for instance to have a `/` in the baseRouteName, everywhere else we use `_`.

It will help me to find the best solutions.

the thing is done using %sonata.page.admin.block.entity% parameter for class and getting the dynamic part, then it uses the function above and adds /shared to it

for why it does it to not collide with the other page admin that uses the same entity as class

VincentLanglet commented 2 years ago

I currently doesn't see a better way than https://github.com/sonata-project/SonataAdminBundle/pull/7867 which would allow the override in PageBundle.

WDYT @jordisala1991 ?

Hanmac commented 2 years ago

i need to run the tests for this again, but the only thing i see are the big Twig change and the Command + Services change.

and both of them already got PRs

VincentLanglet commented 2 years ago

IMHO the command PR need to be restarted since we plan to

For twig there is some changes needed https://github.com/sonata-project/SonataPageBundle/pull/1423

And if you want to help to find a solution, there is this issue https://github.com/sonata-project/SonataPageBundle/issues/1442

Hanmac commented 2 years ago

1x: The "Sonata\Doctrine\Entity\BaseEntityManager::getConnection()" method is deprecated since sonata-project/sonata-doctrine-extensions 1.15 and will be removed in version 2.0. Use "getEntityManager()->getConnection()" instead. 1x in SnapshotManagerTest::testEnableSnapshots from Sonata\PageBundle\Tests\Entity

hm the snapshotmanager uses sql instead of dql or query builder i will see if i can make that better

VincentLanglet commented 2 years ago

On Sql/dql there is this issue https://github.com/sonata-project/SonataPageBundle/issues/1426 dunno if it's the same subject.

Hanmac commented 2 years ago

yeah partly, but also the enable snapshots query

eerison commented 2 years ago

IMHO the command PR need to be restarted since we plan to

For twig there is some changes needed #1423

And if you want to help to find a solution, there is this issue #1442

I'm deprecating the baseCommand in my Pull request ;)

eerison commented 2 years ago

Ok I checked again the 4.x branch and I can see only 6 direct deprecation that is a impediment to support Symfony 5 and sonata 4

VincentLanglet commented 2 years ago

The first two will need something like this on SonataAdmin 4 https://github.com/sonata-project/SonataAdminBundle/pull/7867

Hanmac commented 2 years ago

The "Sonata\Doctrine\Entity\BaseEntityManager::getConnection()"

the Connection thing is part of the Query rework I'm trying to do, i just need to write better testcases

https://github.com/sonata-project/SonataPageBundle/pull/1446

Hanmac commented 2 years ago

1x: The "Sonata\BlockBundle\Block\BlockContextManager" class is considered final since sonata-project/block-bundle 3.0. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Block\BlockContextManager". 1x in BlockContextManagerTest::testGetWithValidData from Sonata\PageBundle\Tests\Block

1x: The "Sonata\BlockBundle\Block\Service\ContainerBlockService" class is considered final since sonata-project/block-bundle 3.0. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Block\ContainerBlockService". 1x in ContainerBlockServiceTest::testExecute from Sonata\PageBundle\Tests\Block

what to do with these two? the blockbundle ones can't be extended anymore. Are the page bundle ones really needed?

otherwise i will think about something

VincentLanglet commented 2 years ago

For the second class,

For the first class, the simpler solution I see is to provide an AbstractBlockContextManager in BlockBundle with everything final except protected configureSettings. Then it will be extended by BlockBundle and PageBundle differently.

Hanmac commented 2 years ago

yeah i was thinking about decoration too

also PageBundle still points to 3.x Block Bundle and not 4.x, but that might be work for another PR

VincentLanglet commented 2 years ago

yeah i was thinking about decoration too

also PageBundle still points to 3.x Block Bundle and not 4.x, but that might be work for another PR

Two solutions:

If it's not too much work, I prefer the second solution, this avoid a release on an old branch.

VincentLanglet commented 2 years ago

Seems like there is https://github.com/sonata-project/SonataPageBundle/pull/1448 for the block deprecations. Help on the review is appreciated @Hanmac :)

jordisala1991 commented 2 years ago 
    public function configure()
    {
        $this->baseRoutePattern = sprintf('%s/%s', $this->getBaseRoutePattern(), 'shared');
        $this->baseRouteName = sprintf('%s/%s', $this->getBaseRouteName(), 'shared');
    }

What about:

    public function configure()
    {
        $this->baseRoutePattern = sprintf('%s/%s', parent::getBaseRoutePattern(), 'shared');
        $this->baseRouteName = sprintf('%s/%s', parent::getBaseRouteName(), 'shared');
    }

Can you try with that? @Hanmac

Hanmac commented 2 years ago 
    public function configure()
    {
        $this->baseRoutePattern = sprintf('%s/%s', $this->getBaseRoutePattern(), 'shared');
        $this->baseRouteName = sprintf('%s/%s', $this->getBaseRouteName(), 'shared');
    }

What about:

    public function configure()
    {
        $this->baseRoutePattern = sprintf('%s/%s', parent::getBaseRoutePattern(), 'shared');
        $this->baseRouteName = sprintf('%s/%s', parent::getBaseRouteName(), 'shared');
    }

Can you try with that? @Hanmac

i didn't tried yet, but it doesn't work because calling getBaseRoutePattern would cache the result and then setting baseRoutePattern wouldn't have any effect

jordisala1991 commented 2 years ago

I just tried, it cant be done without the Pr opened by @VincentLanglet . So this deprecation will be only fixed on 4.x

eerison commented 2 years ago

I will work on those two deprecations

https://github.com/sonata-project/SonataPageBundle/pull/1470 1x: The "Sonata\AdminBundle\Admin\AbstractAdmin::getBaseRoutePattern()" method is considered final since sonata-project/admin-bundle 3.102. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Admin\SharedBlockAdmin". 1x in BaseBlockAdminTest::testSettingAsEditedOnPreBatchDeleteAction from Sonata\PageBundle\Tests\Admin

https://github.com/sonata-project/SonataPageBundle/pull/1470 1x: The "Sonata\AdminBundle\Admin\AbstractAdmin::getBaseRouteName()" method is considered final since sonata-project/admin-bundle 3.102. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Admin\SharedBlockAdmin". 1x in BaseBlockAdminTest::testSettingAsEditedOnPreBatchDeleteAction from Sonata\PageBundle\Tests\Admin

eerison commented 2 years ago

I just tried, it cant be done without the Pr opened by @VincentLanglet . So this deprecation will be only fixed on 4.x

tried what? I don't get it!

VincentLanglet commented 2 years ago

I just tried, it cant be done without the Pr opened by @VincentLanglet . So this deprecation will be only fixed on 4.x

tried what? I don't get it!

You currently cannot solve the deprecations, we need to finish and release https://github.com/sonata-project/SonataAdminBundle/pull/7867. And this will only be solve in 4.x.

The issue is that you cannot override

    public function getBaseRoutePattern()
    {
        return sprintf('%s/%s', parent::getBaseRoutePattern(), 'shared');
    }
    public function getBaseRouteName()
    {
        return sprintf('%s/%s', parent::getBaseRouteName(), 'shared');
    }
eerison commented 2 years ago

I just tried, it cant be done without the Pr opened by @VincentLanglet . So this deprecation will be only fixed on 4.x

tried what? I don't get it!

You currently cannot solve the deprecations, we need to finish and release sonata-project/SonataAdminBundle#7867. And this will only be solve in 4.x.

The issue is that you cannot override

    public function getBaseRoutePattern()
    {
        return sprintf('%s/%s', parent::getBaseRoutePattern(), 'shared');
    }
    public function getBaseRouteName()
    {
        return sprintf('%s/%s', parent::getBaseRouteName(), 'shared');
    }

Yeah it's the issue, and when this PR pass, should I be able to override getBaseRoutePattern?

Edit 1: and your PR will fix all those deprecations that are missing?

Hanmac commented 2 years ago

1x: The "Sonata\BlockBundle\Block\BlockContextManager" class is considered final since sonata-project/block-bundle 3.0. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Block\BlockContextManager". 1x in BlockContextManagerTest::testGetWithValidData from Sonata\PageBundle\Tests\Block 1x: The "Sonata\BlockBundle\Block\Service\ContainerBlockService" class is considered final since sonata-project/block-bundle 3.0. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Block\ContainerBlockService". 1x in ContainerBlockServiceTest::testExecute from Sonata\PageBundle\Tests\Block

this will be fixed by #1448

VincentLanglet commented 2 years ago

Yeah it's the issue, and when this PR pass, should I be able to override getBaseRoutePattern?

You'll be able to override generateBaseRoutePattern. But only in the same PR than the one which bump the SonataAdmin dependency. So it's all or nothing.

eerison commented 2 years ago

I guess it's just missing to add sonata admin 4 to solve this deprecation

1x: The "Sonata\AdminBundle\Admin\AbstractAdmin::getBaseRoutePattern()" method is considered final since sonata-project/admin-bundle 3.102. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Admin\SharedBlockAdmin". 1x in BaseBlockAdminTest::testSettingAsEditedOnPreBatchDeleteAction from Sonata\PageBundle\Tests\Admin

VincentLanglet commented 2 years ago

It requires a SonataAdmin release first.

VincentLanglet commented 2 years ago

I guess it's just missing to add sonata admin 4 to solve this deprecation

1x: The "Sonata\AdminBundle\Admin\AbstractAdmin::getBaseRoutePattern()" method is considered final since sonata-project/admin-bundle 3.102. It may change without further notice as of its next major version. You should not extend it from "Sonata\PageBundle\Admin\SharedBlockAdmin". 1x in BaseBlockAdminTest::testSettingAsEditedOnPreBatchDeleteAction from Sonata\PageBundle\Tests\Admin

Will be done with https://github.com/sonata-project/SonataPageBundle/pull/1485 so we can be close this