Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Routing] Remove variadic constructor signature#46157

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:6.1fromwouterj:pr-45803/backendenum
Apr 27, 2022

Conversation

@wouterj
Copy link
Member

@wouterjwouterj commentedApr 25, 2022•
edited
Loading

QA
Branch?6.1
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

Just a little suggestion, based on today's "New in Symfony" blogpost.

Subjective argument: the variadic signature felt a bit weird, as argument 1 has a different meaning than 2+ (but they look very similar due to PHP's::class "magic constant")
Objective argument: variadic signature is harder with BC layers when we want to add another argument. Personally, I feel like we have to be careful about using variadic signatures in Symfony.

@chalasr
Copy link
Member

Can you check if the build failure is related?

@wouterj
Copy link
MemberAuthor

wouterj commentedApr 25, 2022•
edited
Loading

Fixed fabbot, the CI test failures are caused by changes in PHP 8.2 upstream. You can see the same errors in e.g.https://github.com/symfony/symfony/runs/6146660539

Btw, let's mark PHP 8.2 tests as experimental again:#46160

@nicolas-grekas
Copy link
Member

The benefit of using variadic was the type check enforced by PHP.
I'm really fine doing that but I'd suggest going one step further and allow only one argument: array|string $cases
When array, we should enforce that all cases have the same class.

@fancyweb
Copy link
Contributor

I'm really fine doing that but I'd suggest going one step further and allow only one argument: array|string $cases

👍 or 👎?

@wouterj
Copy link
MemberAuthor

👍 forstring|array - just updated the PR

fancyweb reacted with thumbs up emoji

@fancyweb
Copy link
Contributor

It's actually\BackedEnum|array, I updated it.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I updated the PR to my proposal)

@fabpot
Copy link
Member

Thank you@wouterj.

* @var string[]
*/
privatereadonlyarray$values;
privatestring$requirement;
Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolas-grekas Why did you remove the readonly?

Choose a reason for hiding this comment

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

Not intentional! (But read-only on private is not useful either so 🤷‍♂️ 😆)

@wouterjwouterj deleted the pr-45803/backendenum branchApril 28, 2022 13:04
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof approved these changes

@derrabusderrabusderrabus approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@fancywebfancywebfancyweb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

8 participants

@wouterj@chalasr@nicolas-grekas@fancyweb@fabpot@stof@derrabus@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp