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

[SecurityBundle] Allow specifying attributes forRequestMatcher#45907

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.2fromfreiondrej-lmc:patch-1
Oct 20, 2022

Conversation

@freiondrej-lmc
Copy link
Contributor

@freiondrej-lmcfreiondrej-lmc commentedApr 1, 2022
edited
Loading

QA
Branch?6.1
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#45901
LicenseMIT

The \Symfony\Component\HttpFoundation\RequestMatcher supports array $attributes, which makes it possible to specify a _route (useful e.g. in a multilingual project where $path is translated). However, its current configuration does not offer the possibility to specify the attributes. This PR adds this possibility, so this already existing feature can be leveraged.
I also added a shortcut to just specify "route": "xxx", which translates to "attributes": ["_route": "xxx"].

GromNaN 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

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot
Copy link

Hey!

I think@monteiro has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@carsonbotcarsonbot changed the title#45901 Allow specifying attributes for RequestMatcher[SecurityBundle] #45901 Allow specifying attributes for RequestMatcherApr 3, 2022
@nicolas-grekasnicolas-grekas changed the title[SecurityBundle] #45901 Allow specifying attributes for RequestMatcher[SecurityBundle] Allow specifying attributes for RequestMatcherApr 3, 2022
@nicolas-grekasnicolas-grekas modified the milestones:5.4,6.1Apr 3, 2022
@nicolas-grekas
Copy link
Member

Thanks for the PR.

Please rebase and target 6.1 since that's a new feature. Please also update the description of the PR a bit so that ppl that wonder about what the attached patch does can have some clues (eg to start writing the doc about this).
Then, please add a line in the CHANGELOG file of the bundle, and please add a test case if possible 🙏

freiondrej-lmc reacted with thumbs up emoji

@freiondrej-lmc
Copy link
ContributorAuthor

@carsonbot please find me a reviewer

@chalasr
Copy link
Member

@freiondrej-lmc Thanks for the PR!

@carsonbot please find me a reviewer

The 6.1 branch is feature-frozen, as such the current focus should be about stabilizing 6.1. So this is for 6.2 and we have 6 months to review and fine-tune it.

freiondrej-lmc reacted with thumbs up emoji

@chalasrchalasr removed this from the6.1 milestoneApr 23, 2022
@OskarStarkOskarStark changed the title[SecurityBundle] Allow specifying attributes for RequestMatcher[SecurityBundle] Allow specifying attributes forRequestMatcherSep 21, 2022
@OskarStark
Copy link
Contributor

I don't really understand why the tests fail now, when prior to my fixup commit4983fd5 they passed and I don't see how it could have broken it ... any suggestions would be very welcome 🙂

I think you can ignore the failing test case, its from another PR

@freiondrej-lmc
Copy link
ContributorAuthor

I don't really understand why the tests fail now, when prior to my fixup commit4983fd5 they passed and I don't see how it could have broken it ... any suggestions would be very welcome 🙂

I think you can ignore the failing test case, its from another PR

@OskarStark

Well most of it is, but also mine:

1) Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\SecurityExtensionTest::testRegisterAccessControlWithSpecifiedAttributes[961](https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016303268#step:8:1006)Failed asserting that an array has the key 4.[962](https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016303268#step:8:1007)[963](https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016303268#step:8:1008)/home/runner/work/symfony/symfony/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php:360[964](https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016303268#step:8:1009)[965](https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016303268#step:8:1010)2) Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\SecurityExtensionTest::testRegisterAccessControlWithSpecifiedRoute[966](https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016303268#step:8:1011)Failed asserting that an array has the key 4.

https://github.com/symfony/symfony/actions/runs/3098415653/jobs/5016302933

@OskarStark
Copy link
Contributor

Indeed, sorry I just saw the following:
CleanShot 2022-09-21 at 23 11 16@2x

@freiondrej-lmc
Copy link
ContributorAuthor

@OskarStark I now ran the tests locally and I did not get this error with my code. Maybe there are some changes from a different PR involved? I don’t know the details of the pipeline and how isolated the PRs are from each other 😕 any help will be much appreciated!

@freiondrej-lmc
Copy link
ContributorAuthor

@OskarStark any ideas would be very welcome. Will this PR even make it into 6.2?

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.

(test failures are unrelated)

@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 19, 2022
@fabpot
Copy link
Member

Thank you@freiondrej-lmc.

freiondrej-lmc reacted with heart emoji

@xabbuh
Copy link
Member

@freiondrej-lmc The remaining failures that you mentioned in#45907 (comment) were related to the refactoring of the request matchers that happened in the meantime in#47595.#47930 is going to fix them.

freiondrej-lmc reacted with thumbs up emoji

chalasr added a commit that referenced this pull requestOct 23, 2022
…te matcher (wouterj)This PR was merged into the 6.2 branch.Discussion----------[SecurityBundle] Add XML support for new request attribute matcher| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -This adds support for the XML format to the new attribute request matching options (ref#45907).Commits-------aa64204 [SecurityBundle] Add XML support for new request attribute matcher
@fabpotfabpot mentioned this pull requestOct 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

@OskarStarkOskarStarkAwaiting requested review from OskarStark

Assignees

No one assigned

Labels

FeatureSecurityBundle❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

[SecurityBundle] Allow specifying attributes._route for RequestMatcher

9 participants

@freiondrej-lmc@carsonbot@nicolas-grekas@chalasr@stof@OskarStark@fabpot@xabbuh@welcoMattic

[8]ページ先頭

©2009-2025 Movatter.jp