Improve error message in case of multiple admins for same entity #6486
Conversation
| @@ -1487,6 +1489,18 @@ public function attachAdminClass(FieldDescriptionInterface $fieldDescription) | |||
|
|
|||
| if (!$pool->hasAdminByClass($targetModel)) { | |||
There was a problem hiding this comment.
Can't we use the hasAdminByClass method for this? IMHO the pool should now allow multiple admins for one entity
There was a problem hiding this comment.
How would you do this @core23 ?
There was a problem hiding this comment.
Im my project we have about 10 admin classes for one entity.
Also im think that method getAdminByClass must be deprecated
There was a problem hiding this comment.
Im my project we have about 10 admin classes for one entity.
That's not an issue. I have multiple classes for only one entity too.
Also im think that method getAdminByClass must be deprecated
The admin_code would be required in that case then. Don't know the implication.
And there is another usage of the adminByClass method.
I don't see a bad thing to automatically get an admin by Class, if possible.
There was a problem hiding this comment.
Im discover this method using for getting admin in twig function: sonata_urlsafeid (it seams too old method) and for attaching admins
There was a problem hiding this comment.
So. getAdminByClass can be be removed. Use getAdminByCode
There was a problem hiding this comment.
And attachAdminClass will throw own exception with suggest for define option admin_code.
There was a problem hiding this comment.
We could indeed deprecate getUrlSafeIdentifier in the TwigExtension. Do you mind creating the PR ?
But I'm against the deprecation of getAdminByClass, it's very useful when you only have one admin by class. In these case, there is no need to know, and set, the admin code for OtO or OtM relation between entities.
There was a problem hiding this comment.
We should kept method getAdminClass as legacy?
There was a problem hiding this comment.
For ex: (Instead of getAdminByClass)
VincentLanglet
force-pushed
the
multipleAdmin
branch
2 times, most recently
from
a6534bc to
49daf1a
Compare
src/Admin/AbstractAdmin.php
Outdated
|
|
||
| if (!$pool->hasSingleAdminByClass($targetModel)) { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'To many admin found for the class "%s", please use the admin_code option instead', |
There was a problem hiding this comment.
You check on not single but says that too many( If someone delete method above here message logic will be broken.
There was a problem hiding this comment.
I don't understand, because the method above is not suppose to be deleted.
VincentLanglet
force-pushed
the
multipleAdmin
branch
from
49daf1a to
55f1219
Compare
It's not about adding exception. It's about having custom message more explicit in this method, to avoid issue like #6481
|
VincentLanglet
changed the title
|
|
||
| if (!$pool->hasSingleAdminByClass($targetModel)) { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'Too many admins found for the class "%s", please use the admin_code option instead', |
There was a problem hiding this comment.
We still don't require 7.3, but maybe we could start adopting multiline exception messages anyway
| 'Too many admins found for the class "%s", please use the admin_code option instead', | |
| <<<'EXCEPTION' | |
| Context: trying to find an admin for class "%s" | |
| Problem: That class has more than one admin. | |
| Solution: Use the unambiguous admin_code option instead. | |
| EXCEPTION, |
There was a problem hiding this comment.
Won't it lead to weird display on 7.2 ?
I would prefer to do this change in a more general way on our code ? Maybe even with an helper to generate an exception message ?
public static function formatExceptionMessage($context, $problem, $solution)
Subject
This is BC
Closes #6181
Changelog