sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

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

Closed eerison closed 2 years ago

eerison commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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: https://github.com/sonata-project/SonataAdminBundle/blob/89cac165682046f0fb4b59d1662902f0304aef03/src/Controller/CRUDController.php#L242-L257

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 https://github.com/sonata-project/SonataAdminBundle/blob/89cac165682046f0fb4b59d1662902f0304aef03/src/Controller/CRUDController.php#L174-L177

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

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

WDYT ?

eerison commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I'm doing it in https://github.com/sonata-project/SonataAdminBundle/pull/7761 then.