8000 [WebLink] Added missing types by tarlepp · Pull Request #41507 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WebLink] Added missing types #41507

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

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2021
Merged

[WebLink] Added missing types #41507

merged 1 commit into from
Jun 3, 2021

Conversation

tarlepp
Copy link
Contributor
@tarlepp tarlepp commented Jun 2, 2021
Q A
Branch? 6.0
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR

@tarlepp tarlepp requested a review from dunglas as a code owner June 2, 2021 05:16
@carsonbot carsonbot added this to the 6.0 milestone Jun 2, 2021
@tarlepp tarlepp changed the title Added missing types [WebLink] Added missing types Jun 2, 2021
@carsonbot carsonbot changed the title [WebLink] Added missing types Added missing types Jun 2, 2021
@carsonbot carsonbot changed the title Added missing types [WebLink] Added missing types Jun 2, 2021
Nyholm
Nyholm reviewed Jun 2, 2021
View reviewed changes
src/Symfony/Component/WebLink/Link.php
@@ -134,7 +134,7 @@ public function withoutRel($rel)
*
* @return static
*/
public function withAttribute($attribute, $value)
public function withAttribute(string $attribute, string|\Stringable|int|float|bool|array $value)
{
$that = clone $this;
$that->attributes[$attribute] = $value;
Copy link
Member

Choose a reason for hiding this comment

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

Lets update the type for the $attribute property too. It currently says it should be an array of strings.

@Nyholm
Copy link
Member
Nyholm commented Jun 2, 2021

Thank you.

I see this error on Travis:

Declaration of Symfony\Component\WebLink\GenericLinkProvider::getLinksByRel(string $rel): array must be compatible with Psr\Link\LinkProviderInterface::getLinksByRel($rel) in /home/travis/build/symfony/symfony/src/Symfony/Component/WebLink/GenericLinkProvider.php on line 49

It is the same issue as this: https://3v4l.org/uM5S9

Let's update psr/link to support only ^1.1. Both the root composer.json and the composer.json in the WebLink component. Travis should be happy then.

@tarlepp
Copy link
Contributor Author
tarlepp commented Jun 2, 2021

Let's update psr/link to support only ^1.1. Both the root composer.json and the composer.json in the WebLink component. Travis should be happy then.

Done

derrabus
derrabus reviewed Jun 2, 2021
View reviewed changes
src/Symfony/Component/WebLink/composer.json
@@ -20,7 +20,7 @@
},
"require": {
"php": ">=8.0.2",
"psr/link": "^1.0"
"psr/link": "^1.1"
Copy link
Member

Choose a reason for hiding this comment

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

Shall we add a conflict rule for ext-psr? If we bump here, the component will break if (any released version of) ext-psr is installed.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't care, that's not our problem

@nicolas-grekas
Copy link
Member

Thank you @tarlepp.

@nicolas-grekas nicolas-grekas merged commit bf7201e into symfony:6.0 Jun 3, 2021
@tarlepp tarlepp deleted the weblink-types branch June 3, 2021 14:17
@derrabus derrabus mentioned this pull request Jun 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@Nyholm Nyholm Nyholm left review comments

@derrabus derrabus derrabus left review comments

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
Status: Reviewed WebLink
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tarlepp @Nyholm @nicolas-grekas @dunglas @derrabus @carsonbot
0