Remove Block context manager

[VincentLanglet]

Subject

I am targeting this branch, because {reason}.

Closes #{put_issue_number_here}.

Changelog

### Removed
- Remove `BlockContextManager` service to use the default one from BlockBundle.

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[eerison]

Hey @VincentLanglet do you need that I test it? But what exactly do you need that I test :D

[VincentLanglet]

In #1495 you reported that you got the error

at http://localhost:8001/admin/dashboard is returning An exception has been thrown during the rendering of a template ("The options "manager", "page_id" do not exist. Defined options are: "attr", "extra_cache_keys", "groups", "template", "ttl", "use_cache".").

I was wondering if my PR fixed the issue (or if you get another error which would explain why this class is useful)

[eerison]

In #1495 you reported that you got the error

at http://localhost:8001/admin/dashboard is returning An exception has been thrown during the rendering of a template ("The options "manager", "page_id" do not exist. Defined options are: "attr", "extra_cache_keys", "groups", "template", "ttl", "use_cache".").

I was wondering if my PR fixed the issue (or if you get another error which would explain why this class is useful)

ohh Ok I got it, I'll check ;)

[eerison]

Hey @VincentLanglet when I tried to install your branch I got this error

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In ContainerBuilder.php line 1030:
!!                                                                                 
!!    You have requested a non-existent service "sonata.page.block.context_manage  
!!    r".                                                                          
!!                                                                                 
!!  
!!  
Script @auto-scripts was called via post-update-cmd

[VincentLanglet]

Hey @VincentLanglet when I tried to install your branch I got this error

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In ContainerBuilder.php line 1030:
!!                                                                                 
!!    You have requested a non-existent service "sonata.page.block.context_manage  
!!    r".                                                                          
!!                                                                                 
!!  
!!  
Script @auto-scripts was called via post-update-cmd

Do you have the config

sonata_block:
    context_manager: sonata.page.block.context_manager

?

I removed it from the doc, because I removed the service, i think you have to use the block manager from Block bundle so
sonata.block.context_manager.default (or just remove the line since it's the default value).

[eerison]

Hey @VincentLanglet when I tried to install your branch I got this error

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In ContainerBuilder.php line 1030:
!!                                                                                 
!!    You have requested a non-existent service "sonata.page.block.context_manage  
!!    r".                                                                          
!!                                                                                 
!!  
!!  
Script @auto-scripts was called via post-update-cmd

Do you have the config

sonata_block:
    context_manager: sonata.page.block.context_manager

?

I removed it from the doc, because I removed the service, i think you have to use the block manager from Block bundle so sonata.block.context_manager.default (or just remove the line since it's the default value).

Hey @VincentLanglet I removed this line context_manager: sonata.page.block.context_manager from my sonata_page.yaml, and I didn't get any issue with composer update

and my admin page is working as expect

I don't know why were we using those "manager", "page_id", but it looks fine 👀

[eerison]

Hey @VincentLanglet should we add this note about

context_manager: sonata.page.block.context_manager? In upgrade notes? Or it's not necessary?

[VincentLanglet]

Hey @VincentLanglet should we add this note about

context_manager: sonata.page.block.context_manager? In upgrade notes? Or it's not necessary?

Indeed. We should even deprecate the context_manager in the 3.x branch (and maybe we dont even need it on 3.x).

So a PR on 3.x is needed. Do you want to do it or I do it ?

[eerison]

Hey @VincentLanglet should we add this note about
context_manager: sonata.page.block.context_manager? In upgrade notes? Or it's not necessary?

Indeed. We should even deprecate the context_manager in the 3.x branch (and maybe we dont even need it on 3.x).

So a PR on 3.x is needed. Do you want to do it or I do it ?

Hey @VincentLanglet well for the free time that I'm getting I'm investing in this template issue :/

Then i can't do that for now :/

[eerison]

@VincentLanglet FYI: when I try to use 3.x code without context_manager: sonata.page.block.context_manager

I got this error

 [KO]
Script cache:clear returned with error code 255
!!  
!!  Fatal error: Class App\Entity\SonataPagePage contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Sonata\PageBundle\Model\PageInterface::getId) in /app/src/Entity/SonataPagePage.php on line 16

Sorry It is related with other thing!

[eerison]

Hey @VincentLanglet could you wait the Serializer PR pass? because after that I can test a page with blocks, I guess this functionality is used when the page has blocks! and for now it's not possible to test because it's broken :/

Because in 3.x if I remove this option (context_manager: sonata.page.block.context_manager)

I get this error

[VincentLanglet]

Hey @VincentLanglet could you wait the Serializer PR pass? because after that I can test a page with blocks, I guess this functionality is used when the page has blocks! and for now it's not possible to test because it's broken :/

Because in 3.x if I remove this option (context_manager: sonata.page.block.context_manager)

I get this error

It could be interesting to know more about this error ; which class is complaining about the manager/page_id option ?

Looking at https://github.com/sonata-project/SonataPageBundle/search?p=1&q=page_id, I wonder if it's not cache related ; and since we remove this feature...

[eerison]

Hey @VincentLanglet could you wait the Serializer PR pass? because after that I can test a page with blocks, I guess this functionality is used when the page has blocks! and for now it's not possible to test because it's broken :/
Because in 3.x if I remove this option (context_manager: sonata.page.block.context_manager)
I get this error

It could be interesting to know more about this error ; which class is complaining about the manager/page_id option ?

Looking at https://github.com/sonata-project/SonataPageBundle/search?p=1&q=page_id, I wonder if it's not cache related ; and since we remove this feature...

I'll try to clean up my cache, I'll post here the results :)

[VincentLanglet]

I'll try to clean up my cache, I'll post here the results :)

I meant it's maybe related to class inside the src/Cache folder.
Since we removed all this classes in 4.x it may explain why we don't need this class anymore.

Anyway, I'll wait for the other PR to be merged first

[eerison]

I'll try to clean up my cache, I'll post here the results :)

I meant it's maybe related to class inside the src/Cache folder. Since we removed all this classes in 4.x it may explain why we don't need this class anymore.

Anyway, I'll wait for the other PR to be merged first

ohhh I got it!

yeah But I can't test my contact page yet, because of this issue #1496

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

I'll try to clean up my cache, I'll post here the results :)

I meant it's maybe related to class inside the src/Cache folder. Since we removed all this classes in 4.x it may explain why we don't need this class anymore.
Anyway, I'll wait for the other PR to be merged first

ohhh I got it!

yeah But I can't test my contact page yet, because of this issue #1496

Pr is rebased if you want to try again @eerison

[eerison]

It works @VincentLanglet 🥳

[VincentLanglet]

WDYT @jordisala1991 Should we go with this strategy ?

If so, I am not sure it really useful to introduce some deprecation in 3.x. if the option (context_manager: sonata.page.block.context_manager) cannot be removed without breaking the app.

[jordisala1991]

I its broken, IMO lets fix on 4.x by removing it, and lets see if someone complains. It seems to work and the branch is not released, so its not a big risk, we can recover that file later if needed. And I dont think we should do a deprecation here.

[eerison]

WDYT @jordisala1991 Should we go with this strategy ?

If so, I am not sure it really useful to introduce some deprecation in 3.x. if the option (context_manager: sonata.page.block.context_manager) cannot be removed without breaking the app.

I guess we could go with this, and make the 4.x branch usable, and when we going to do a final test we can try to cover this functionality, I can add this in the 4.0 tickets list!

[eerison]

I its broken, IMO lets fix on 4.x by removing it, and lets see if someone complains. It seems to work and the branch is not released, so its not a big risk, we can recover that file later if needed. And I dont think we should do a deprecation here.

exactly

[VincentLanglet]

I its broken, IMO lets fix on 4.x by removing it, and lets see if someone complains. It seems to work and the branch is not released, so its not a big risk, we can recover that file later if needed. And I dont think we should do a deprecation here.

Ready to review then @jordisala1991

[jordisala1991]

OK for the code, IMO we could add some entry in the UPGRADE guide. to mention this was removed.

[VincentLanglet]

OK for the code, IMO we could add some entry in the UPGRADE guide. to mention this was removed.

Added