Closed yleyvag closed 3 years ago
phansys
commented
3 years ago I think we have 2 choices here:
ModelManagerInterface::hasNormalizedIdentifier();ModelManagerInterface::getNormalizedIdentifier() to return null for new or removed models.@sonata-project/contributors, WDYT?
VincentLanglet
commented
3 years ago I think we have 2 choices here:
- Add a
ModelManagerInterface::hasNormalizedIdentifier();- Let
ModelManagerInterface::getNormalizedIdentifier()to returnnullfor new or removed models.@sonata-project/contributors, WDYT?
getNormalizedIdentifier is used a lot:
In AbstractAdmin
public function getNormalizedIdentifier(object $model): string
{
return $this->getModelManager()->getNormalizedIdentifier($model);
}
public function id(object $model): string
{
return $this->getNormalizedIdentifier($model);
}And then the id method is also use multiple times.
IMHO the simpler solution would be to allow null as a return value but I don't know the impact.
phansys
commented
3 years ago IMHO the simpler solution would be to allow null as a return value but I don't know the impact.
I think there is no critical impact. I've introduced this deprecation and exception in sonata-project/SonataDoctrineORMAdminBundle#1049, in order to narrow the API and avoid invalid values, but if this case can be considered legit, we could allow null as return type.
SilberMa
commented
3 years ago Does this mean that I am not able to create a new object until the milestone is reached?
I tried to allow null as return in the AbstractAdmin and the Interface, but this doesn't change anything. :/
Furthermore adding an id (in my case Uuid) in preValidate has no impact.
VincentLanglet
commented
3 years ago Does this mean that I am not able to create a new object until the milestone is reached?
Using the master branch is at your own risk, we provide stable 3.x branch and a unstable master.
We're putting a lot of effort working on Sonata 4. There is currently lot of BC-break on master, and some bug can appear. We appreciate a lot that you report the bugs you encounter with the master branch but can't guarantee to always solve quickly the bugs.
If you want to provide PR, I'll be happy to review it and merge it.
I tried to allow
nullas return in the AbstractAdmin and the Interface, but this doesn't change anything. :/Furthermore adding an id (in my case Uuid) in preValidate has no impact.
Your exception is thrown by https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/master/src/Model/ModelManager.php#L392
You can see that previously it was returning null
https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Model/ModelManager.php#L452
In your case, the state 2 is the state NEW.
The call is made by AbstractAdmin::id. The question is why ?
The answer is...
public function isGranted($name, ?object $object = null): bool
{
$objectRef = $object ? sprintf('/%s#%s', spl_object_hash($object), $this->id($object)) : '';
$key = md5(json_encode($name).$objectRef);
if (!\array_key_exists($key, $this->cacheIsGranted)) {
$this->cacheIsGranted[$key] = $this->securityHandler->isGranted($this, $name, $object ?: $this);
}
return $this->cacheIsGranted[$key];
}We could add try/catch, default value, etc...
But when I look at getNormalizedIdentifiers I think it should never throw an exception. If no identifier can be determined, let's just return null or '' instead.
To me, null seems better. Does @sonata-project/contributors agree ?
So it requires a PR to change
getUrlSafeIdentifier
getNormalizedIdentifier
idreturn type from string to ?string in AbstractAdmin and AdminInterface/UrlGeneratorInterface.
Then (in the same PR)
getUrlSafeIdentifier
getNormalizedIdentifierreturn type from string to ?string in ModelManagerInterface
Then a PR in SonataDoctrineORM 3.x to remove the deprecation https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Model/ModelManager.php#L456-L469
And a PR in SonataDoctrineORM master to remove the exception and return null instead.
You could add also the check
if (0 === \count($values)) {
return null;
}again.
SilberMa
commented
3 years ago Using the master branch is at your own risk, we provide stable 3.x branch and a unstable master.
We're putting a lot of effort working on Sonata 4. There is currently lot of BC-break on master, and some bug can appear. We appreciate a lot that you report the bugs you encounter with the master branch but can't guarantee to always solve quickly the bugs.
You're totally right and I wanna say thank you for your effort explaining everything.
A PR is a good idea and I'll try to provide one.
Kind regards.
VincentLanglet
commented
3 years ago Should be fixed now.
Environment
Sonata packages
Symfony packages
PHP version
Subject
Can not get the normalized identifier for App\Entity\BettingEventCategory since it is in state 2.
Steps to reproduce
BettingEventCategory class
BettingEventCategoryAdmin class
My service section
Trying to create a new entity
Expected results
The new entity should be created
Actual results
Error received