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 listener when try to remove a data that is related with other entity #7759

Closed
eerison opened this issue Feb 28, 2022 · 7 comments · Fixed by #7761
Closed

Add listener when try to remove a data that is related with other entity #7759

eerison opened this issue Feb 28, 2022 · 7 comments · Fixed by #7761
Labels

Comments

@eerison
Copy link
Contributor

eerison commented Feb 28, 2022

Add flash message for ForeignKeyConstraintViolationException

When I try to remove a data that is related with other entity I'm getting an internal server error of ForeignKeyConstraintViolationException

I was thinking to add a listener for this exception and add a flash message with the error!
do you see any problem it be into the sonata code or should it be into my code?

@VincentLanglet
Copy link
Member

Can you give us more details about this exception.
Do you know where it's thrown ?

Because we're already catching ModelManagerThrowable in the CRUDController

Which is supposed to be thrown here: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/4.x/src/Model/ModelManager.php#L96-L108

If we add custom logic, it won't be on sonata admin but on sonataDoctrineORMAdminBundle, since ForeignKeyConstraintViolationException is doctrineOrm related.
And it's already supposed to be caught and throw as a ModelManagerThrowable...

@eerison
Copy link
Contributor Author

eerison commented Mar 2, 2022

Hey @VincentLanglet

it's the exception

Doctrine\DBAL\Exception\
ForeignKeyConstraintViolationException
An exception occurred while executing a query: SQLSTATE[23503]: Foreign key violation: 7 ERROR: update or delete on table "category" violates foreign key constraint "fk_64c19c1727aca70" on table "category" DETAIL: Key (id)=(2) is still referenced from table "category".

Screenshot 2022-03-02 at 09 07 09

sonata packages

sonata-project/admin-bundle              dev-fix-navbar-leftside 77b3337 dev-fix-navbar-leftside 77b3337 The missing Symfony Admin Generator
sonata-project/block-bundle              4.10.0                          4.10.0                          Symfony SonataBlockBundle
sonata-project/cache                     2.2.0                           2.2.0                           Cache library
sonata-project/doctrine-extensions       1.16.0                          1.16.0                          Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 4.2.5                           4.2.6                           Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  2.11.0                          2.11.0                          Lightweight Exporter library
sonata-project/form-extensions           1.13.1                          1.13.1                          Symfony form extensions
sonata-project/twig-extensions           1.9.1                           1.9.1                           Sonata twig extensions

@VincentLanglet
Copy link
Member

@eerison Ok, so it's already caught by Sonata:

The error is caught here: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/595960b96f6a3c234141ffa8bc4e8bb9ea1d15a9/src/Model/ModelManager.php#L102-L107

Then, it's handled here:

} catch (ModelManagerThrowable $e) {
$this->handleModelManagerThrowable($e);
if ($this->isXmlHttpRequest($request)) {
return $this->renderJson(['result' => 'error'], Response::HTTP_OK, []);
}
$this->addFlash(
'sonata_flash_error',
$this->trans(
'flash_delete_error',
['%name%' => $this->escapeHtml($objectName)],
'SonataAdminBundle'
)
);
}

The method handle throwable is here: https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Controller/CRUDController.php#L1083-L1096

In debug, it throws the exception, but in production you just log the error so it will add a flash message "an error occured" with

$this->addFlash(
'sonata_flash_error',
$this->trans('flash_batch_delete_error', [], 'SonataAdminBundle')
);

Maybe what could be change is the fact that the handleModelManagerThrowable could return a string instead of void.

  • If you return null, the default message is used
  • If you return a string, your message is used

Then you could do "If the previous exeption is a ForeignKeyConstraintViolationException" I use my custom message.

WDYT ?

@eerison
Copy link
Contributor Author

eerison commented Mar 2, 2022

Yeah can be, but should be good return something like "This "object" is related with others data." or something like this, because if the user see "an error occured" only, he won't know what is the problem!

@VincentLanglet
Copy link
Member

VincentLanglet commented Mar 2, 2022

Yeah can be, but should be good return something like "This "object" is related with others data." or something like this, because if the user see "an error occured" only, he won't know what is the problem!

You think like a developer. An object or a relation is something talking to you. For some project the admin can be something usable by every one, even customers.
Sending a too much technical message is not a good idea in those case.

So Sonata have to go with a generic message, and will let open the ability to override/change it.

Do you want to make the Pr to allow handleModelManagerThrowable to return a string ?

@eerison
Copy link
Contributor Author

eerison commented Mar 2, 2022

Yeah can be, but should be good return something like "This "object" is related with others data." or something like this, because if the user see "an error occured" only, he won't know what is the problem!

You think like a developer. An object or a relation is something talking to you. For some project the admin can be something usable by every one, even customers. Sending a too much technical message is not a good idea in those case.

So Sonata have to go with a generic message, and will let open the ability to override/change it.

Do you want to make the Pr to allow handleModelManagerThrowable to return a string ?

I see, well I'm working on admin V3, I can't do that now :/

@VincentLanglet
Copy link
Member

I'm doing it in #7761 then.

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 a pull request may close this issue.

2 participants