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

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

Merged
merged 12 commits into from
Sep 26, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
32d138b
Do not trigger errors when a block doesn't exist anymore
jeromeengeln Sep 16, 2022
d073eb3
Restore @throws tags
jeromeengeln Sep 19, 2022
9562a7a
rollback non-useful updates
jeromeengeln Sep 19, 2022
3309860
cs
jeromeengeln Sep 19, 2022
9619f99
cs
jeromeengeln Sep 19, 2022
d06eff4
Add a unit test to verify that BlockNotFoundException is throwed inst…
jeromeengeln Sep 19, 2022
09b63bf
update old testGetBlockServiceException with new BlockNotFoundException
jeromeengeln Sep 19, 2022
1ef7a68
Replacing BlockNotFoundException by new more relevant BlockServiceNot…
jeromeengeln Sep 19, 2022
cd0a0bb
implements BlockExceptionInterface and extends \Exception
jeromeengeln Sep 20, 2022
373de4a
Replace \RuntimeException by new BlockServiceNotFoundException()
jeromeengeln Sep 23, 2022
0000861
Replace \RuntimeException by BlockServiceNotFoundException into test
jeromeengeln Sep 23, 2022
c14664c
Update src/Exception/BlockServiceNotFoundException.php
VincentLanglet Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/Block/BlockServiceManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Psr\Container\ContainerInterface;
use Sonata\BlockBundle\Block\Service\BlockServiceInterface;
use Sonata\BlockBundle\Block\Service\EditableBlockService;
use Sonata\BlockBundle\Exception\BlockServiceNotFoundException;
use Sonata\BlockBundle\Model\BlockInterface;
use Sonata\Form\Validator\ErrorElement;
use Symfony\Component\DependencyInjection\ContainerInterface as DependencyInjectionContainerInterface;
Expand Down Expand Up @@ -190,18 +191,18 @@ public function validate(ErrorElement $errorElement, BlockInterface $block): voi
}

/**
* @throws \RuntimeException
jeromeengeln marked this conversation as resolved.
Show resolved Hide resolved
* @throws BlockServiceNotFoundException
*/
private function load(string $type): BlockServiceInterface
{
if (!$this->has($type)) {
throw new \RuntimeException(sprintf('The block service `%s` does not exist', $type));
throw new BlockServiceNotFoundException(sprintf('The block service `%s` does not exist', $type));
jeromeengeln marked this conversation as resolved.
Show resolved Hide resolved
}

if (!$this->services[$type] instanceof BlockServiceInterface) {
$blockService = $this->container->get($type);
if (!$blockService instanceof BlockServiceInterface) {
throw new \RuntimeException(sprintf('The service %s does not implement BlockServiceInterface', $type));
throw new BlockServiceNotFoundException(sprintf('The service %s does not implement BlockServiceInterface', $type));
}

jeromeengeln marked this conversation as resolved.
Show resolved Hide resolved
$this->services[$type] = $blockService;
Expand Down
18 changes: 18 additions & 0 deletions src/Exception/BlockServiceNotFoundException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

declare(strict_types=1);

/*
* This file is part of the Sonata Project package.
*
* (c) Thomas Rabaix <thomas.rabaix@sonata-project.org>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Sonata\BlockBundle\Exception;

final class BlockServiceNotFoundException extends \RuntimeException implements BlockExceptionInterface
{
Copy link
Contributor

@eerison eerison Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should it extend from http exception 🤔

If yes then we should add NotFoundHttpException as Suffix?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not, I copied the old exception. And what about that ?

namespace Sonata\BlockBundle\Exception;

final class BlockServiceNotFoundException implements BlockExceptionInterface
{
}

Or simply :

final class BlockServiceNotFoundException
{

Let me know and I'll do modifications tomorrow morning

Copy link
Member

Choose a reason for hiding this comment

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

it needs to extend some kind of exception. If you also implement the BlockExceptionInterface maybe thats enough to render empty response automatically? or not?

I feel like the whole exception catching should happen in the block bundle, are you sure you want to introduce specific logic for page @VincentLanglet ?

Copy link
Contributor

@eerison eerison Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like the whole exception catching should happen in the block bundle, are you sure you want to introduce specific logic for page @VincentLanglet ?

Hey @jordisala1991, do you mean here https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Templating/Helper/BlockHelper.php#L388 ?

But if we do this, we can't handle the exception for code that uses this method, if for some reason I need to implement some logical if the block doesn't exist, I can't do that, because I never get the exception!

usually I handle the exception in the last layer before to return to client.

Copy link
Member

Choose a reason for hiding this comment

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

First I dunno why BlockNotFound extends NotFoundException, it feels weird https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Exception/BlockNotFoundException.php#L18

I mean, I don't see how we're in a Http context here:

SonataBlockBundle/src/Block/BlockLoaderChain.php

Line 66 in 375b168

throw new BlockNotFoundException();
and we should throw a 404.

And indeed there is a BlockExceptionInterface.

Seems like a strategy exist to ignore block exceptions https://github.com/sonata-project/SonataBlockBundle/blob/987d2936d3d2a3d4fe4615241f545aad693eb4c2/docs/reference/exceptions.rst#filters

So indeed, throwing a BlockServiceNotFound extends BlockExceptionInterface would allow people to ignore this exception with the correct filter.
It might require some conf in PageBundle still to use the correct filter.

But again, I asked a question and got no answer so far:
which filter and which renderer is using SonataPageBundle.
https://github.com/sonata-project/SonataBlockBundle/blob/987d2936d3d2a3d4fe4615241f545aad693eb4c2/docs/reference/exceptions.rst#exception-strategy

IMHO, blockbundle should allow to ignore them (with specific exception/config) and pagebundle should activate this ; and this already a feature from blockbundle.

Copy link
Contributor

Choose a reason for hiding this comment

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

After @JeromeEngelnAdeliom change it, I'll test in sonata page bundle, and check if those configs works as expected :D

Copy link
Member

Choose a reason for hiding this comment

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

It would also be nice if someone checks why that interface was introduced and why they didn't implement any exception with that interface, maybe there is a reason, maybe its just one more "WTF".

Ten years ago #36 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eerison wdyt about using extends \UnexpectedValueException ? it seems to match with our missing block service case.
=> Exception thrown if a value does not match with a set of values. Typically this happens when a function calls another function and expects the return value to be of a certain type or value not including arithmetic or buffer related errors.

Copy link
Contributor

@eerison eerison Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMO BlockServiceNotFoundException isn't an abstraction of UnexpectedValueException 👀

Copy link
Member

Choose a reason for hiding this comment

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

Extending Exception is enough.

And fun fact, @jordisala1991, https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Exception/BlockExceptionInterface.php#L19 is currently not extending Throwable...

}
5 changes: 3 additions & 2 deletions tests/Block/BlockServiceManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use PHPUnit\Framework\TestCase;
use Sonata\BlockBundle\Block\BlockServiceManager;
use Sonata\BlockBundle\Block\Service\BlockServiceInterface;
use Sonata\BlockBundle\Exception\BlockServiceNotFoundException;
use Sonata\BlockBundle\Model\BlockInterface;
use Symfony\Component\DependencyInjection\Container;

Expand All @@ -39,7 +40,7 @@ public function testGetBlockService(): void

public function testInvalidServiceType(): void
{
$this->expectException(\RuntimeException::class);
$this->expectException(BlockServiceNotFoundException::class);

$service = $this->createMock(\stdClass::class);

Expand All @@ -58,7 +59,7 @@ public function testInvalidServiceType(): void

public function testGetBlockServiceException(): void
{
$this->expectException(\RuntimeException::class);
$this->expectException(BlockServiceNotFoundException::class);

$manager = new BlockServiceManager(new Container(), []);

Expand Down