Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix psalm build #6322

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Fix psalm build #6322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Form/DataTransformer/ModelToIdPropertyTransformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public function __construct(

public function reverseTransform($value)
{
/** @var ArrayCollection<array-key, object> $collection */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's considered as the right way to do. I don't know. I'll open an issue on psalm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you use this plugin: https://github.com/weirdan/doctrine-psalm-plugin ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phansys I found an existing issue and this seems to be the recommended way: vimeo/psalm#3436 (comment)

Copy link
Member

@phansys phansys Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 created ArrayCollection.
  • 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$collection = new ArrayCollection();

if (empty($value)) {
Expand All @@ -97,7 +98,7 @@ public function reverseTransform($value)
continue;
}

$collection[] = $this->modelManager->find($this->className, $id);
$collection->add($this->modelManager->find($this->className, $id));
}

return $collection;
Expand Down
3 changes: 2 additions & 1 deletion src/Form/DataTransformer/ModelsToArrayTransformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,14 @@ public function reverseTransform($keys)
throw new UnexpectedTypeException($keys, 'array');
}

/** @var ArrayCollection<array-key, object> $collection */
$collection = new ArrayCollection();
$notFound = [];

// optimize this into a SELECT WHERE IN query
foreach ($keys as $key) {
if ($model = $this->modelManager->find($this->class, $key)) {
$collection[] = $model;
$collection->add($model);
} else {
$notFound[] = $key;
}
Expand Down