Do not trigger errors when a block doesn't exist anymore #1115

jeromeengeln · 2022-09-19T10:30:26Z

Keep the blocks into the page rendering even if some blocks does not exist

Here a proposition to remove exception on render when a block doesn't not exists anymore.

The purpose of changing exception is to avoid throwing an exception for blocks that does not exist anymore (for example a removed service and still present into page block composition - issue sonata-project/SonataPageBundle#1609 in sonata page) and keep all other runtime errors.
To do that I need to identify not found exception.

if BlockNotFoundException is catched just return an empty response
And for other \RuntimeException just keep old behavior

Related to SonataPageBundle issue sonata-project/SonataPageBundle#1609
And PR sonata-project/SonataPageBundle#1611

Changelog

### Changed
- BlockServiceManager::load throw a BlockServiceNotFoundException when the block is not found.

sonata-project/SonataPageBundle#1611 sonata-project/SonataPageBundle#1609

eerison · 2022-09-19T11:15:02Z

~~I was working in this issue as well 😄~~

~~But I don't get why you need to change those class?~~

~~I just needed to change PageExtension~~

sorry I misunderstand your PR ;)

eerison · 2022-09-19T11:16:03Z

can you provide tests please!

jeromeengeln · 2022-09-19T11:39:00Z

can you provide tests please!

you mean create a new unit test for those changes ? I'm not really familiar with that practice but if you guide me I can try

jordisala1991 · 2022-09-19T11:42:52Z

src/Block/BlockRenderer.php

@@ -79,6 +82,10 @@ public function render(BlockContextInterface $blockContext, ?Response $response
                ), compact('exception'));
            }

+            if ($exception instanceof RuntimeError && !empty($exception->getPrevious()) && $exception->getPrevious() instanceof BlockNotFoundException) {


I don't think this code belongs here. We have ExceptionStrategyManager for that I guess.

Yes this could be here instead https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Exception/Strategy/StrategyManager.php#L107-L125

which would rely on https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Exception/Strategy/StrategyManager.php#L113-L114

So maybe it's just a blockRenderer/filter to update in BlockBundle or Pagebundle

src/Block/BlockServiceManager.php

VincentLanglet · 2022-09-19T12:02:11Z

can you provide tests please!

you mean create a new unit test for those changes ? I'm not really familiar with that practice but if you guide me I can try

This can be a functional test (might be better).

The current issue for me is to understand the issue you and @eerison are trying to fix.
So best would be

A commit to reproduce the error with functional tests (which means a failing build)
A commit with your fix (and the tests will pass)

Because currently I'm like "Why should be have a different treatment for those errors ?"

Also, looking at the code (cf #1115 (comment)) it seems like this could be already configured to not throw an exception but render some error message...

Also I don't see why it solve sonata-project/SonataPageBundle#1609 and replace https://github.com/sonata-project/SonataPageBundle/pull/1611/files

jeromeengeln · 2022-09-19T12:33:14Z

Because currently I'm like "Why should be have a different treatment for those errors ?"

Yeah the purpose of changing exception is to avoid throwing an exception for blocks that does not exist anymore (for example a removed service and still present into page block composition - issue 1609 in sonata page) and keep all other runtime errors.
To do that I need to identify not found exception.

if BlockNotFoundException is catched just return an empty response
And for other \RuntimeException just keep old behavior

eerison · 2022-09-19T12:58:21Z

A commit to reproduce the error with functional tests (which means a failing build)
A commit with your fix (and the tests will pass)

Yep!

VincentLanglet · 2022-09-19T13:03:40Z

Because currently I'm like "Why should be have a different treatment for those errors ?"

Yeah the purpose of changing exception is to avoid throwing an exception for blocks that does not exist anymore (for example a removed service and still present into page block composition - issue 1609 in sonata page) and keep all other runtime errors. To do that I need to identify not found exception.

if BlockNotFoundException is catched just return an empty response

And for other \RuntimeException just keep old behavior

Doing a PR to throws BlockNotFoundException instead of \RuntimeException seems fine to me.
We already use BlockNotFoundException in some places.

The main question is about the way we handle BlockNotFoundException.
-> Should be handled differently in BlockBundle and why ?
-> Should be handle differently in PageBundle instead and why ?

A failing (functional) test would help to understand the why.

We have multiple renderer https://github.com/sonata-project/SonataBlockBundle/tree/4.x/src/Exception/Renderer and filter https://github.com/sonata-project/SonataBlockBundle/tree/4.x/src/Exception/Filter ; might be interesting to know which ones are used in PageBundle that would help to understand why an exception is thrown.

jeromeengeln · 2022-09-19T13:04:09Z

Also I don't see why it solve sonata-project/SonataPageBundle#1609 and replace https://github.com/sonata-project/SonataPageBundle/pull/1611/files

For sonata-project/SonataPageBundle#1609
It allow to avoid the exception mentionned in issue (the front-end part) :

An exception has been thrown during the rendering of a template ("The block service `sonata.seo.block.email.share_button` does not exist").

And the PR sonata-project/SonataPageBundle#1611 just fix and improve the sonata page compose view behavior when some block has been removed (not throw 500 on block list and allow to remove even if the block does not exists anymore). So it's merly related in our discussions with @eerison

VincentLanglet · 2022-09-19T13:12:18Z

Also I don't see why it solve sonata-project/SonataPageBundle#1609 and replace https://github.com/sonata-project/SonataPageBundle/pull/1611/files

For sonata-project/SonataPageBundle#1609 It allow to avoid the exception mentionned in issue (the front-end part) :
An exception has been thrown during the rendering of a template ("The block service `sonata.seo.block.email.share_button` does not exist").

Yes I looked at the code and kinda understand more the issue.
SonataPage is using the BlockHelper from BlockBundle to render the block which is throwing the exception.

But since there is an exceptionManagerStrategy, we can use a different one.
Or basically just add a try/catch (BlockNotFoundException) here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Twig/Extension/PageExtension.php#L236

So there are multiple way to solve this and we need to precisely decide if it must be fix in SonataPage or SonataBock.
I'm tempted to think that the best would be to

Use BlockNotFoundException in BlockBundle but do not touch the way BlockHelper works
Solve the issue in SonataPage by changing the exceptionRendererStrategy or adding a try catch if another exceptionRendererStrategy doesn't make sens.

jeromeengeln · 2022-09-19T13:14:29Z

The main question is about the way we handle BlockNotFoundException.
-> Should be handled differently in BlockBundle and why ?
-> Should be handle differently in PageBundle instead and why ?

I see the idea, I'll think about it.

eerison · 2022-09-19T13:14:48Z

Or basically just add a try/catch (BlockNotFoundException) here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Twig/Extension/PageExtension.php#L236

Yep, I'm doing exactly this now :)

eerison · 2022-09-19T13:16:25Z

to solve the issue related with page (when open the page it's returning internal server error), I just need this line

https://github.com/sonata-project/SonataBlockBundle/pull/1115/files#diff-26110fbfbdde2b86990d5fc9dd60c1f629a5688cb6bb0932f0f9cdd5bbfb4de9R200

eerison · 2022-09-19T13:16:43Z

to solve the issue related with page (when open the page it's returning internal server error), I just need this line

https://github.com/sonata-project/SonataBlockBundle/pull/1115/files#diff-26110fbfbdde2b86990d5fc9dd60c1f629a5688cb6bb0932f0f9cdd5bbfb4de9R200

~~But should be good~~ We must write a test for it!

VincentLanglet · 2022-09-19T13:19:34Z

to solve the issue related with page (when open the page it's returning internal server error), I just need this line

https://github.com/sonata-project/SonataBlockBundle/pull/1115/files#diff-26110fbfbdde2b86990d5fc9dd60c1f629a5688cb6bb0932f0f9cdd5bbfb4de9R200

Yeah, I think this PR should be reduce to just changing some \RuntimeException to BlockNotFoundException
Wdyt @jordisala1991 @JeromeEngelnAdeliom ?

jeromeengeln · 2022-09-19T13:23:53Z

to solve the issue related with page (when open the page it's returning internal server error), I just need this line
https://github.com/sonata-project/SonataBlockBundle/pull/1115/files#diff-26110fbfbdde2b86990d5fc9dd60c1f629a5688cb6bb0932f0f9cdd5bbfb4de9R200

Yeah, I think this PR should be reduce to just changing some \RuntimeException to BlockNotFoundException Wdyt @jordisala1991 @JeromeEngelnAdeliom ?

Fine for me ! I did the rollback commit

eerison · 2022-09-19T13:24:55Z

src/Block/BlockServiceManager.php

     */
    private function load(string $type): BlockServiceInterface
    {
        if (!$this->has($type)) {
-            throw new \RuntimeException(sprintf('The block service `%s` does not exist', $type));
+            throw new BlockNotFoundException(sprintf('The block service `%s` does not exist', $type));


A unit test is required hire!

Yeah, I'll try to do it

I added a test, If you have some comments to improve do not hesitate or the test is not well organized

src/Block/BlockServiceManager.php

…ead of \RuntimeError

VincentLanglet · 2022-09-22T13:30:52Z

well But IMO make sense to add a try catch here: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L390 I don't see a problem to add a try/catch there 👀

We can catch it directly here: #1117
Does it solve your issue ?

eerison · 2022-09-22T13:31:21Z

private function resolve(BlockInterface $block, array $settings): array
    {
        $optionsResolver = new OptionsResolver();

        $this->configureSettings($optionsResolver, $block);

        try {
            $service = $this->blockService->get($block);
            $service->configureSettings($optionsResolver);
        } catch (\Exception) {
            // Todo
        }

        return $optionsResolver->resolve($settings);
    }

well I can add the try catch here, But the point is: Can I add the catch like this: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/BlockRenderer.php#L73-L86 👀

eerison · 2022-09-22T13:33:04Z

well But IMO make sense to add a try catch here: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L390 I don't see a problem to add a try/catch there 👀

We can catch it directly here: #1117 Does it solve your issue ?

it solves, But the info that BlockService doesn't exist will be hidden,

as I said above, can we add this

https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/BlockRenderer.php#L73-L86

eerison · 2022-09-22T13:35:09Z

well But IMO make sense to add a try catch here: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L390 I don't see a problem to add a try/catch there 👀

We can catch it directly here: #1117 Does it solve your issue ?

it solves, But the info that BlockService doesn't exist will be hidden,

as I say above, can we add this

https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/BlockRenderer.php#L73-L86

and other thing, if Block and BlockService are different, then BlockServiceNotFoundException should not implements BlockNotFoundInterface

VincentLanglet · 2022-09-22T13:41:08Z

well But IMO make sense to add a try catch here: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L390 I don't see a problem to add a try/catch there 👀

We can catch it directly here: #1117 Does it solve your issue ?

it solves, But the info that BlockService doesn't exist will be hidden,

as I said above, can we add this

https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/BlockRenderer.php#L73-L86

I can add log like this 17f93fc

But the exceptionManagerStrategy should not be used since it's supposed to return a Response and we won't use it.

eerison · 2022-09-22T13:47:33Z

well But IMO make sense to add a try catch here: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L390 I don't see a problem to add a try/catch there 👀

We can catch it directly here: #1117 Does it solve your issue ?

it solves, But the info that BlockService doesn't exist will be hidden,
as I said above, can we add this
https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/BlockRenderer.php#L73-L86

I can add log like this 17f93fc

But the exceptionManagerStrategy should not be used since it's supposed to return a Response and we won't use it.

Yeah it is a bit better then do not log anything 😄

But I guess it should not require exception, it should only handle exception and nothing more!

eerison · 2022-09-22T13:50:18Z

well But IMO make sense to add a try catch here: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L390 I don't see a problem to add a try/catch there 👀

We can catch it directly here: #1117 Does it solve your issue ?

it solves, But the info that BlockService doesn't exist will be hidden,
as I said above, can we add this
https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Block/BlockRenderer.php#L73-L86

I can add log like this 17f93fc
But the exceptionManagerStrategy should not be used since it's supposed to return a Response and we won't use it.

Yeah it is a bit better then do not log anything 😄

But I guess it should not require exception, it should only handle exception and nothing more!

But well it's requiring in the interface and it'll be too much work for now :/
https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Exception/Strategy/StrategyManagerInterface.php#L33

src/Block/BlockServiceManager.php

VincentLanglet · 2022-09-23T10:04:15Z

As explained here: #1117 (comment), this PR works fine with the config

sonata_block:
    default_contexts: [sonata_page_bundle]
    exception:
        default:
            filter: ignore_block_exception

Also, by default it's debug only ; so it won't throw any exception in production. So I think this is not a real issue.

eerison · 2022-09-23T10:47:17Z

Also, by default it's debug only ; so it won't throw any exception in production. So I think this is not a real issue.

But the page doesn't render the others blocks

eerison · 2022-09-23T10:54:05Z

I'm using the same config that you added above and my page is still broken

VincentLanglet · 2022-09-23T11:41:50Z

Also, by default it's debug only ; so it won't throw any exception in production. So I think this is not a real issue.

But the page doesn't render the others blocks

Yes but this is not an issue.
You decided it should display other blocks but why ? The page might not have any sens anymore if you hide one block.
Having this default behavior is not choking ; then people can change the strategy, like I did

VincentLanglet · 2022-09-23T11:42:17Z

@JeromeEngelnAdeliom Can you update the tests too ? =)

eerison · 2022-09-23T11:49:17Z

The page might not have any sens anymore if you hide one block.

do you mean if one block fail, my whole page no make sense anymore? I don't get it!

VincentLanglet · 2022-09-23T12:02:44Z

The page might not have any sens anymore if you hide one block.

do you mean if one block fail, my whole page no make sense anymore? I don't get it!

Yes it can.

Let's say you have a block Text "Please subscribe by giving your email on the next line".
Then you display a Block Email but this one doesn't exist (so is not displayed).
The first block alone doesn't make any sens.

I can find more and more example. So the current beahvior is understandable ; and if you want a different one you'll be able to use another manager strategy. Imho it require just some doc on SonataPageBundle.

VincentLanglet

The fact that the tests failed mean we introduced a regression. To avoid this we need that BlockServiceNotFoundException extends RuntimeException

src/Exception/BlockServiceNotFoundException.php

eerison · 2022-09-23T12:28:07Z

The page might not have any sens anymore if you hide one block.

do you mean if one block fail, my whole page no make sense anymore? I don't get it!

Yes it can.

Let's say you have a block Text "Please subscribe by giving your email on the next line". Then you display a Block Email but this one doesn't exist (so is not displayed). The first block alone doesn't make any sens.

I can find more and more example. So the current beahvior is understandable ; and if you want a different one you'll be able to use another manager strategy. Imho it require just some doc on SonataPageBundle.

Well it really depends of each case

in my case some pages uses blocks completely independent each other, and if one block fail, make totally sense for me still keep my page showing with others blocks. But if it's not an impediment for sonata page 4 then It's fine

VincentLanglet · 2022-09-23T12:47:37Z

in my case some pages uses blocks completely independent each other, and if one block fail, make totally sense for me still keep my page showing with others blocks.

And because it depends of each case, each developper should be able to decide what to do.
AND that the job of the configuration of the exception manager strategy. I dunno how many times I have to say it.

jordisala1991

The PR looks ok to me, but I can't follow all the conversation. I let you decide if this is ready to merge @VincentLanglet

VincentLanglet · 2022-09-26T07:51:28Z

The PR looks ok to me, but I can't follow all the conversation. I let you decide if this is ready to merge @VincentLanglet

To me, it is. And this works fine for me. Dunno why @eerison doesn't made it work for him.

Do not trigger errors when a block doesn't exist anymore

32d138b

sonata-project/SonataPageBundle#1611 sonata-project/SonataPageBundle#1609

jeromeengeln mentioned this pull request Sep 19, 2022

PR - Proposition for issue #1609 sonata-project/SonataPageBundle#1611

Closed

jordisala1991 reviewed Sep 19, 2022

View reviewed changes

VincentLanglet reviewed Sep 19, 2022

View reviewed changes

src/Block/BlockServiceManager.php Show resolved Hide resolved

Restore @throws tags

d073eb3

rollback non-useful updates

9562a7a

eerison reviewed Sep 19, 2022

View reviewed changes

eerison suggested changes Sep 19, 2022

View reviewed changes

src/Block/BlockServiceManager.php Show resolved Hide resolved

jeromeengeln added 2 commits September 19, 2022 15:27

cs

3309860

cs

9619f99

VincentLanglet previously approved these changes Sep 19, 2022

View reviewed changes

VincentLanglet requested review from jordisala1991 and eerison September 19, 2022 14:18

Add a unit test to verify that BlockNotFoundException is throwed inst…

d06eff4

…ead of \RuntimeError

jeromeengeln dismissed VincentLanglet’s stale review via d06eff4 September 19, 2022 14:23

VincentLanglet mentioned this pull request Sep 22, 2022

Try #1117

Closed

VincentLanglet requested changes Sep 23, 2022

View reviewed changes

src/Block/BlockServiceManager.php Show resolved Hide resolved

src/Block/BlockServiceManager.php Outdated Show resolved Hide resolved

Replace \RuntimeException by new BlockServiceNotFoundException()

373de4a

jeromeengeln dismissed eerison’s stale review via 373de4a September 23, 2022 10:22

VincentLanglet previously approved these changes Sep 23, 2022

View reviewed changes

Replace \RuntimeException by BlockServiceNotFoundException into test

0000861

jeromeengeln dismissed VincentLanglet’s stale review via 0000861 September 23, 2022 12:09

VincentLanglet reviewed Sep 23, 2022

View reviewed changes

src/Exception/BlockServiceNotFoundException.php Outdated Show resolved Hide resolved

Update src/Exception/BlockServiceNotFoundException.php

c14664c

VincentLanglet approved these changes Sep 23, 2022

View reviewed changes

VincentLanglet requested a review from jordisala1991 September 23, 2022 12:12

jordisala1991 approved these changes Sep 26, 2022

View reviewed changes

VincentLanglet merged commit 4cf3211 into sonata-project:4.x Sep 26, 2022

Do not trigger errors when a block doesn't exist anymore #1115

Do not trigger errors when a block doesn't exist anymore #1115

Conversation

jeromeengeln commented Sep 19, 2022 • edited by VincentLanglet Loading

Keep the blocks into the page rendering even if some blocks does not exist

Changelog

eerison commented Sep 19, 2022 • edited Loading

eerison commented Sep 19, 2022

jeromeengeln commented Sep 19, 2022

jordisala1991 Sep 19, 2022

Choose a reason for hiding this comment

VincentLanglet Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

VincentLanglet commented Sep 19, 2022 • edited Loading

jeromeengeln commented Sep 19, 2022

eerison commented Sep 19, 2022

VincentLanglet commented Sep 19, 2022

jeromeengeln commented Sep 19, 2022

VincentLanglet commented Sep 19, 2022

jeromeengeln commented Sep 19, 2022

eerison commented Sep 19, 2022 • edited Loading

eerison commented Sep 19, 2022

eerison commented Sep 19, 2022 • edited Loading

VincentLanglet commented Sep 19, 2022

jeromeengeln commented Sep 19, 2022 • edited Loading

eerison Sep 19, 2022

Choose a reason for hiding this comment

jeromeengeln Sep 19, 2022

Choose a reason for hiding this comment

jeromeengeln Sep 19, 2022

Choose a reason for hiding this comment

VincentLanglet commented Sep 22, 2022

eerison commented Sep 22, 2022

eerison commented Sep 22, 2022 • edited Loading

eerison commented Sep 22, 2022 • edited Loading

VincentLanglet commented Sep 22, 2022

eerison commented Sep 22, 2022 • edited Loading

eerison commented Sep 22, 2022

VincentLanglet commented Sep 23, 2022 • edited Loading

eerison commented Sep 23, 2022

eerison commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

eerison commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

VincentLanglet left a comment

Choose a reason for hiding this comment

eerison commented Sep 23, 2022

VincentLanglet commented Sep 23, 2022

jordisala1991 left a comment

Choose a reason for hiding this comment

VincentLanglet commented Sep 26, 2022

jeromeengeln commented Sep 19, 2022 •

edited by VincentLanglet

Loading

eerison commented Sep 19, 2022 •

edited

Loading

VincentLanglet Sep 19, 2022 •

edited

Loading

VincentLanglet commented Sep 19, 2022 •

edited

Loading

eerison commented Sep 19, 2022 •

edited

Loading

eerison commented Sep 19, 2022 •

edited

Loading

jeromeengeln commented Sep 19, 2022 •

edited

Loading

eerison commented Sep 22, 2022 •

edited

Loading

eerison commented Sep 22, 2022 •

edited

Loading

eerison commented Sep 22, 2022 •

edited

Loading

VincentLanglet commented Sep 23, 2022 •

edited

Loading