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.

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

Some Phpstan fix for level 6 #7176

Merged
merged 5 commits into from May 23, 2021

Conversation

VincentLanglet
Copy link
Member

No description provided.

Comment on lines 27 to 29
if (null === $value) {
return null;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The transform method were supporting null (and returning null in this case).
So if the transform support FilterData|null => array<string, mixed>|null,
I assume the reverseTransport should support array<string, mixed>|null => FilterData|null.

@@ -90,7 +90,7 @@ public function __construct(
*/
public function reverseTransform($value)
{
if (empty($value)) {
if (null === $value || [] === $value || '' === $value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This avoid issue with 0 or '0', and I don't see any reason to support false.

@@ -99,6 +99,10 @@ public function reverseTransform($value)
}

if (!$this->multiple) {
if (\is_array($value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

find only support string|int.

*
* @phpstan-return T|null
*/
public function reverseTransform($value): ?object
{
if (empty($value) && !\in_array($value, ['0', 0], true)) {
if (null === $value || '' === $value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

find only supports string|int so I don't see reason to support [] or false.

@@ -53,11 +53,11 @@ public function __construct(ModelManagerInterface $modelManager, string $class)
}

/**
* @param object[]|null $value
* @param \Traversable<object>|null $value
Copy link
Member Author

Choose a reason for hiding this comment

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

reverseTransform returns a Collection.

@@ -93,7 +95,6 @@ public function reverseTransform($value)
if ([] === $value) {
$result = $value;
} else {
$value = array_map('strval', $value);
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 don't think this is useful. addIdentifiersToQuery supports both int and string.
And we shouldn't support any other value since transform return a string[].

@@ -150,7 +150,7 @@ public function executeQuery(object $query);
public function getExportFields(string $class): array;

/**
* @param array<int, int|string> $idx
* @param array<int|string> $idx
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 don't see reason to restrict the keys.

@VincentLanglet VincentLanglet marked this pull request as ready for review May 8, 2021 11:11
@VincentLanglet VincentLanglet requested a review from a team May 8, 2021 11:11
continue;
}

$value[$key] = (string) $id;
Copy link
Member Author

Choose a reason for hiding this comment

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

addIdentifiersToQuery supports both int and string so it's not needed to cast it.

@VincentLanglet VincentLanglet added this to the 4.0 milestone May 9, 2021
@VincentLanglet
Copy link
Member Author

Please take a look @sonata-project/contributors

@VincentLanglet
Copy link
Member Author

Friendly ping @sonata-project/contributors

franmomu
franmomu previously approved these changes May 23, 2021
@@ -30,8 +37,10 @@ public function reverseTransform($value): FilterData

/**
* @param FilterData|null $value
*
* @return array<string, mixed>|null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return array<string, mixed>|null
* @return array<string, mixed>|null
* @phpstan-return array{type: int|null, value?: mixed}|null

@@ -68,6 +70,9 @@ public function testReverseTransform(?array $value): void
}
}

/**
* @return iterable<array{array<int|string>|null}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return iterable<array{array<int|string>|null}>
* @phpstan-return iterable<array{array<int|string>|null}>

@VincentLanglet VincentLanglet merged commit ee0287b into sonata-project:master May 23, 2021
@VincentLanglet VincentLanglet deleted the phpstanFixFor6 branch May 23, 2021 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants