Closed jaikdean closed 3 years ago
VincentLanglet
commented
4 years ago The typehint is correct. toString expects to be only used with object.
It currently trigger a deprecation, but will throw an exception in 4.0.
In the example you get, there is a check $admin->hasSubject(), so the subject can't be null.
Any other example ?
jaikdean
commented
4 years ago The static analysis isn't configured to know that hasSubject() returning true means that getSubject() will return object, as things could change inbetween the calls. We use Psalm on our projects rather than PHPStan, but it interprets the PHPStan typehints and rightly complains about this. For example the following code results in null being passed to toString():
if ($admin->hasSubject()) {
$admin->setSubject(null);
$admin->toString($admin->getSubject());
}The AbstractAdmin::toString() implementation still accepts null as an argument. It throws a deprecation warning, but passing null is still allowed by this method in 3.x.
GetShortObjectDescriptionAction::__invoke() has a code path that results in calling toString(null) for JSON responses.
VincentLanglet
commented
4 years ago The static analysis isn't configured to know that
hasSubject()returningtruemeans thatgetSubject()will returnobject, as things could change inbetween the calls. We use Psalm on our projects rather than PHPStan, but it interprets the PHPStan typehints and rightly complains about this. For example the following code results innullbeing passed totoString():if ($admin->hasSubject()) { $admin->setSubject(null); $admin->toString($admin->getSubject()); }
$admin->getSubject() will return object in 4.0 and throw an exception if there is no object, so you should rely on hasSubject.
GetShortObjectDescriptionAction::__invoke()has a code path that results in callingtoString(null)for JSON responses.
As explained in the comment of this method, in next major it will return a NotFound exception.
throw new NotFoundHttpException(sprintf('Could not find subject for id "%s"', $objectId));I still see no issue with the toString method.
mixed and return '' if it's not an objectobject in next major. What's why we recommend to use it only with object, or the user will have a BC-break in next major.jaikdean
commented
4 years ago I understand the deprecations and upcoming BC-breaks, but these typehints are in 3.x releases. The typehint for toString() in 3.x says it only accepts object, but that's not true. The deprecation warnings should indicate upcoming deprecations, the typehints should document what the methods currently accept, not recommendations for the future.
Perhaps it's a difference between how PHPStan and Psalm work, but in Psalm the typehint annotations define the interface for the method, they're not just suggestions. After updating our project from 3.75.0 to 3.76.0, we got a huge pile of type errors because of these changes.
VincentLanglet
commented
4 years ago I understand the deprecations and upcoming BC-breaks, but these typehints are in 3.x releases. The typehint for
toString()in 3.x says it only acceptsobject, but that's not true. The deprecation warnings should indicate upcoming deprecations, the typehints should document what the methods currently accept, not recommendations for the future.
If you really document what the methods currently accept, you should use mixed, since we're returning '' value then.
But that doesn't help the user to correctly use this function.
If I have a function
/**
* @param int $int
*/
function addOne($int) {
return $int + 1;
}You could say in a similar way that int is not a correct typehint, since it can work with others value.
Perhaps it's a difference between how PHPStan and Psalm work, but in Psalm the typehint annotations define the interface for the method, they're not just suggestions. After updating our project from 3.75.0 to 3.76.0, we got a huge pile of type errors because of these changes.
Currently you talked about BreadcrumbsBuilder and GetShortObjectDescriptionAction. If there are some Psalm errors, they should not coming from Sonata code, but from yours. Do you have any example ?
For instance,
if ($admin->hasSubject()) {
\assert(null !== $admin->getSubject());
$admin->toString($admin->getSubject());
}is one way to fix them.
I also had multiple error from Phpstan when updating the Sonata version, but it was coming from the introduction of generics.
Environment
Sonata packages
Symfony packages
PHP version
Subject
3.76.0 introduced a number of typehints in annotations. The argument for
AdminInterface::toString($object)is typehinted asobject, but is sometimes passednull. For example,BreadcrumbsBuilderpasses the returned value fromAdminInterface::getSubject(), which is typehinted asobject|null.