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] Fix localized paths#40767

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
derrabus merged 1 commit intosymfony:5.xfroml-vo:fix_localized_paths
May 2, 2021
Merged

Conversation

@l-vo
Copy link
Contributor

QA
Branch?5.x
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
Doc PR

Related to#40266, localized paths does not work anymore. This PR aims to fix that and add a rework on the tests (using real annotations/attributes instead of guessing the parsing that may lead to errors).

COil reacted with thumbs up emoji
@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@l-vol-voforce-pushed thefix_localized_paths branch from58ca2b4 to1e20cd6CompareApril 11, 2021 10:04
@l-vol-vo marked this pull request as ready for reviewApril 11, 2021 10:04
@carsonbotcarsonbot changed the titleWIP [Routing] Fix localized paths[Routing] WIP Fix localized pathsApr 11, 2021
@l-vol-voforce-pushed thefix_localized_paths branch from1e20cd6 to67fd37fCompareApril 11, 2021 10:07
@l-vol-vo changed the title[Routing] WIP Fix localized paths[Routing] Fix localized pathsApr 11, 2021
@nicolas-grekasnicolas-grekas modified the milestones:4.4,5.xApr 11, 2021
Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

Good catch, thank you for your PR!

}

/**
* @Route({"requirements":{"locale":"en"}, "path":""})
Copy link
Member

Choose a reason for hiding this comment

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

Okay, right. This does work now that we've enabled@NamedArgumentConstructor, but I'd consider this to be an accident rather than intended behavior. I would not build a test case for that.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

related tohttps://github.com/symfony/symfony/pull/40767/files#r612721813. This fixture is to test deprecation but maybe I misunderstand the target of the deprecation.

Copy link
Member

@derrabusderrabusApr 13, 2021•
edited
Loading

Choose a reason for hiding this comment

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

The deprecation is not supposed to be triggered by normal annotation usage. The target are manual constructor calls (that should not happen anyway).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Oh ok thank you. I pushed some changes :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sorry, I used cs fixer with a wrong PHP version;it broke attributes. I'll fix it tomorrow.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Updated :)

@l-vol-voforce-pushed thefix_localized_paths branch 4 times, most recently fromd061b3d toec47816CompareApril 13, 2021 21:28
@l-vol-voforce-pushed thefix_localized_paths branch from4b97084 to9bf4a24CompareApril 14, 2021 07:00
@l-vo
Copy link
ContributorAuthor

status: needs review

@COil
Copy link
Contributor

COil commentedMay 2, 2021•
edited
Loading

Hello, is there something to check related to this issue? I saw it wasn't shipped with 5.3 beta2.

@derrabus
Copy link
Member

IIRC, we're only waiting for a second core team approval.

COil reacted with thumbs up emoji

Copy link
Contributor

@TobionTobion left a comment

Choose a reason for hiding this comment

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

The fix seems fine. But I think the deprecation in#40266 is not complete.

@derrabus
Copy link
Member

Thank you@l-vo.

@derrabusderrabus merged commit21e3738 intosymfony:5.xMay 2, 2021
@fabpotfabpot mentioned this pull requestMay 9, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus approved these changes

+1 more reviewer

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.3

Development

Successfully merging this pull request may close these issues.

6 participants

@l-vo@carsonbot@COil@derrabus@Tobion@nicolas-grekas
[8]ページ先頭
©2009-2025 Movatter.jp