Fix psalm build #6322
Fix psalm build #6322
Conversation
VincentLanglet
force-pushed
the
fixPsalm
branch
2 times, most recently
from
08480b9 to
717e2ad
Compare
| @@ -74,6 +74,7 @@ public function __construct( | |||
|
|
|||
| public function reverseTransform($value) | |||
| { | |||
| /** @var ArrayCollection<array-key, object> $collection */ | |||
There was a problem hiding this comment.
IMHO, inlining hints for 3rd party dependencies doesn't seem a good choice.
If that's the solution, I'd prefer to allow the failure indicating the cause. That way, when the dependency fixes the issue, we only need to remove the entry for that failure.
There was a problem hiding this comment.
Maybe it's considered as the right way to do. I don't know. I'll open an issue on psalm.
There was a problem hiding this comment.
What happens if you use this plugin: https://github.com/weirdan/doctrine-psalm-plugin ?
There was a problem hiding this comment.
@phansys I found an existing issue and this seems to be the recommended way: vimeo/psalm#3436 (comment)
There was a problem hiding this comment.
Surely declaring locally a type that is missing in a dependency will fix the issues caused by that dependency. My point is, we should not create such a compatibility layer for these cases, and instead, let them to fail, declaring these failures as allowed in our own list (psalm-baseline.xml).
There was a problem hiding this comment.
I'm not sure to understand.
If you ignore the error in psalm-baseline,
- either you'll have to add a new entry every time a
add()is called on a new createdArrayCollection. - either you will ignore error like
$collection->add(1)
It's not a type that is missing in a dependency and Psalm does not consider this situation as a bug:
When you write new ArrayCollection() you're setting no elements, so the inferred type is ArrayCollection<array-key, empty>`.
IMHO it's not compatibility layer, it's declaring the right information.
If $collection was a class property, you would see no issue writting
/**
* @var Collection<array-key, object>
*/
$collection
If it was a param property, you would see no issue writting
/**
* @param Collection<array-key, object>
*/
public function foo($collection)
Here it's the same to me.
There was a problem hiding this comment.
On the previous code, relying on the modelManager we would have written return type like that, so I am okay with the solution, we should probably end up using templates on this function because the object depends on the class string passed on the construct
There was a problem hiding this comment.
It's fine. I don't like the idea to provide specific information for a tool in such intrinsic way. I'd expect the tool to leverage the types it knows and let the other cases without any warning.
If the tool doesn't consider this as a problem solvable with a definition change in the package providing the offending method, I think it's our responsibility to fix the issue, since the decision to add the tool, being aware about this situation, was ours.
VincentLanglet
force-pushed
the
fixPsalm
branch
from
717e2ad to
b791dfd
Compare
|
@jordisala1991 you added the patch label but it is pedantic to me. Do you agree ? |
|
Yes, Agree |
|
Thank you @VincentLanglet |
Subject
I am targeting this branch, because BC.
Pedantic