-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
base: 7.3
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Ok, I removed the attribute and only kept the new phpdoc, prompted in #60420 (comment) |
#[NoDiscard]
and phpdoc types/** | ||
* Returns a list of attributes that describe the target URI. | ||
* | ||
* @return array<string, scalar|\Stringable|list<scalar|\Stringable>> |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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>>
There was a problem hiding this comment.
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.
Make the
array
type more specific in PHPdoc.