Skip to content

WIP update Stan to lvl6#1

Open
iskelic wants to merge 20 commits into
developfrom
refactor/update-phpstan
Open

WIP update Stan to lvl6#1
iskelic wants to merge 20 commits into
developfrom
refactor/update-phpstan

Conversation

@iskelic

@iskelic iskelic commented Mar 29, 2021

Copy link
Copy Markdown

Description:

Updating PHPStan to lvl6

@iskelic iskelic requested a review from penovic March 29, 2021 09:43
@iskelic iskelic self-assigned this Mar 29, 2021
@KondukterCRO KondukterCRO force-pushed the refactor/update-phpstan branch from 8b1baff to c5870e3 Compare February 9, 2022 17:18
@ghost

ghost commented Mar 22, 2022

Copy link
Copy Markdown

@penovic @iskelic I tried this one out, phpstan is all green, is there any further work on this one, perhaps adding psalm?

@KondukterCRO KondukterCRO changed the base branch from master to develop May 3, 2023 12:26
@ghost ghost requested review from a user and KondukterCRO and removed request for penovic October 25, 2023 11:43
Comment thread phpstan.neon Outdated
/**
* @return array<string,mixed>
*
* @psalm-suppress PossiblyUnusedMethod

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@mplesa1 this method is used inside the project, how come Psalm recognizes it as unused?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Used inside tests

*
* @throws JsonApiEncodingException
*
* @psalm-suppress PossiblyUnusedMethod

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Used inside tests?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Even though it's used, Psalm recognizes it as unused?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I removed the annotation because it's also used inside PhpArrayToRelationshipCollectionEncoder. But yes, if it's only used inside some test we need annotation.

Comment thread src/Definition/Encoding/PhpArrayToRelationshipCollectionEncoderInterface.php Outdated
Comment thread src/Definition/Exception/Request/UnsupportedFilterAttributeGivenException.php Outdated
Comment thread src/Definition/Model/Request/CreateResourceRequestInterface.php
Comment thread src/Definition/Model/Request/ResourcePayloadRequest.php
Comment thread src/Definition/Model/Request/UpdateResourceRequestInterface.php
Comment thread src/Implementation/Encoding/PhpArrayToResourceEncoder.php
Comment thread src/Implementation/Model/Link/LinkCollection.php Outdated
@mplesa1

mplesa1 commented Oct 25, 2023

Copy link
Copy Markdown

@penovic @iskelic I tried this one out, phpstan is all green, is there any further work on this one, perhaps adding psalm?

psalm is added


use Undabot\JsonApi\Definition\Model\Resource\ResourceInterface;

/** @psalm-suppress UnusedClass */

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Psalm documentation says the following:

If this class is used and part of the public API, annotate it with @psalm-api.

I believe we should check all of the classes that are used as part of json-api-symfony and mark them with @psalm-api instead of suppressing the error.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good catch, I replace it

$rawMeta = $resource['meta'] ?? null;
$rawLink = $resource['links']['self'] ?? null;

$rawAttributes = $this->assertStringKeyArray($resource['attributes'] ?? []);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What do you think about adding a string $key to this method, resulting in the coalescing operator being moved to the assertStringKeyArray method to avoid doing it outside on each call to the method?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

agree, good catch, I refactored both assertStringKeyArray and assertStringKeyArrayNested

Comment on lines +96 to +116
private function assertStringKeyArray(mixed $array): array
{
if (!is_array($array)) {
throw new \InvalidArgumentException("Parameter is not array.");
}
foreach ($array as $key) {
if (!is_string($key)) {
throw new \InvalidArgumentException("Array key must be a string.");
}
}

return $array;

}

/**
*
* @param mixed $array
* @return array<string, array<string, mixed>>
*/
function assertStringKeyArrayNested(mixed $array): array

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These methods could be moved to some sort of a utility class if we're going to need them in more places?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, I move them to the existing abstract class ArrayUtil

class SortSet implements \IteratorAggregate
{
/** @var Sort[] */
private $sorts;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

array?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

{
/** @var Attribute[] */
/** @var AttributeInterface[] */
private $attributes;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

array?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

{
/** @var array */
/** @var RelationshipInterface[] */
private $relationships = [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

array?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

done

Comment on lines +28 to +30
// $message = sprintf('ResourceIdentifierInterface expected, %s given', \get_class($resourceIdentifier));
//
// throw new InvalidArgumentException($message);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't know, @KondukterCRO did those changes in this branch d05ac8b#diff-390909c32070ee11ae2baba70ca5c9e766a3b2bf1a3a2297dd63ddde7a79f4db Pena, do you have some information?

Comment on lines +24 to +26
// $message = sprintf('ResourceIdentifierInterface expected, %s given', \get_class($resourceIdentifier));
//
// throw new InvalidArgumentException($message);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same question as before

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@KondukterCRO same question as before

interface LinkToPhpArrayEncoderInterface
{
/**
* @return null|array<string,mixed>|string

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants