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.

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

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented May 15, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

Make the array type more specific in PHPdoc.

@nicolas-grekas

This comment was marked as outdated.

@GromNaN
Copy link
Member Author

GromNaN commented May 15, 2025

Ok, I removed the attribute and only kept the new phpdoc, prompted in #60420 (comment)

@GromNaN GromNaN changed the title [WebLink] Add #[NoDiscard] and phpdoc types [WebLink] Specify Link attributes types May 15, 2025
@GromNaN GromNaN changed the title [WebLink] Specify Link attributes types [WebLink] Specify Link attributes types in phpdoc May 15, 2025
@GromNaN GromNaN removed the Feature label May 15, 2025
/**
* 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.

@OskarStark OskarStark changed the title [WebLink] Specify Link attributes types in phpdoc [WebLink] Specify Link attributes types in PHPDoc May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants