8000 [WebLink] Specify Link attributes types in PHPDoc by GromNaN · Pull Request #60429 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WebLink] Specify Link attributes types in PHPDoc #60429

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

Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
+14 −1
Open

[WebLink] Specify Link attributes types in PHPDoc #60429

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
15 changes: 14 additions & 1 deletion src/Symfony/Component/WebLink/Link.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@
private array $rel = [];

/**
* @var array<string, string|bool|string[]>
* @var array<string, scalar|\Stringable|list<scalar|\Stringable>>
*/
private array $attributes = [];

Expand Down Expand Up @@ -175,6 +175,11 @@
return array_values($this->rel);
}

/**
* Returns a list of attributes that describe the target URI.
*
* @return array<string, scalar|\Stringable|list<scalar|\Stringable>>
Copy link
Member

Choose a reason for hiding this comment

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

please also fix the @var of the property, which misses the \Stringable type (and some scalar types).

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 @var type was correct according to LinkInterface:

the value is either a PHP primitive or an array of PHP strings

Copy link
Member

Choose a reason for hiding this comment

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

Then our implementation is broken, as our setter allows setting Stringable but does not cast them to string, which means getAttributes may return non-scalar values.

Copy link
Member

Choose a reason for hiding this comment

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

and then it means that the new type added here is wrong (there is no reason for the private property and its getter to have different types, as the getter returns the property value as is)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should change the implementation to cast stringables to strings.
Stringables are useful to create "lazy strings" and casting too early would break this capability.
We could cast on first read (and possibly memoize).
BUT: what a mess of complexity to fit one bad idea; strict types namely...

I'm fine keeping as proposed personally, or changing only here (keep the argument) to:
@return array<string, scalar|list<scalar>>

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 think the current type is simple and meets needs. I don't know to silent the psalm error.

*/
public function getAttributes(): array
{
return $this->attributes;
Expand Down Expand Up @@ -204,7 +209,15 @@
return $that;
}

/**
* Returns an instance with the specified attribute added.
*
* If the specified attribute is already present, it will be overwritten
* with the new value.
*
* @param scalar|\Stringable|list<scalar|\Stringable> $value
*/
public function withAttribute(string $attribute, string|\Stringable|int|float|bool|array $value): static

Check failure on line 220 in src/Symfony/Component/WebLink/Link.php

View workflow job for this annotation

GitHub Actions / Psalm

MoreSpecificImplementedParamType 

src/Symfony/Component/WebLink/Link.php:220:94: MoreSpecificImplementedParamType: Argument 2 of Symfony\Component\WebLink\Link::withAttribute has the more specific type 'Stringable|list<Stringable|scalar>|scalar', expecting 'Stringable|array<array-key, mixed>|scalar' as defined by Psr\Link\EvolvableLinkInterface::withAttribute (see https://psalm.dev/140)

Check failure on line 220 in src/Symfony/Component/WebLink/Link.php

View workflow job for this annotation

GitHub Actions / Psalm

MoreSpecificImplementedParamType 

src/Symfony/Component/WebLink/Link.php:220:94: MoreSpecificImplementedParamType: Argument 2 of Symfony\Component\WebLink\Link::withAttribute has the more specific type 'Stringable|list<Stringable|scalar>|scalar', expecting 'Stringable|array<array-key, mixed>|scalar' as defined by Psr\Link\EvolvableLinkInterface::withAttribute (see https://psalm.dev/140)
{
$that = clone $this;
$that->attributes[$attribute] = $value;
Expand Down
Loading
0