Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

EZP-31875: Added request matcher to recognize REST requests executed in AdminUI context #1488

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

kmadejski
Copy link
Member

@kmadejski kmadejski commented Sep 16, 2020

Question Answer
Tickets EZP-31875
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes/no
Doc needed? no
License GPL-2.0

This PR introduces a RequestMatcher that is needed for ezsystems/ezplatform-rest#54.

I'm happy to change the class name if someone has any better ideas.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@bdunogier
Copy link
Member

I suggest you change the commit and PR's descriptions to match what that specific PR actually does (e.g. add a REST + Admin request matcher). It can easily get confusing in changelogs. The code itself should probably not refer to JWT either.

Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

+1 but we are missing unit tests coverage.

The code itself should probably not refer to JWT either.

Agree with @bdunogier, matcher could be used in other scenarios as well 😉

@lserwatka
Copy link
Member

@kmadejski we need to add tests as well

@alongosz
Copy link
Member

alongosz commented Oct 2, 2020

This PR introduces a RequestMatcher that is needed for ezsystems/ezplatform-rest#54.

@kmadejski Could you elaborate on that? What does it do?
AFAIK REST has an independent context / routes so not sure why admin context would ever be present there. But I'm probably missing the function here.

@adamwojs
Copy link
Member

adamwojs commented Oct 2, 2020

@kmadejski Could you elaborate on that? What does it do?
AFAIK REST has an independent context / routes so not sure why admin context would ever be present there. But I'm probably missing the function here.

@alongosz See ezsystems/ezplatform-rest#54 (comment)

@alongosz
Copy link
Member

alongosz commented Oct 2, 2020

Ok, +1 once test coverage is present.

@kmadejski kmadejski changed the title EZP-31875: As a developer I want to authenticate against REST API using JWT EZP-31875: Added request matcher to recognize REST requests executed in AdminUI context Oct 3, 2020
@kmadejski
Copy link
Member Author

@alongosz / @webhdx / @ViniTou - added tests, everything is green. Could you please do a final review?

@micszo micszo self-assigned this Oct 6, 2020
@adamwojs adamwojs merged commit a0ff55c into master Oct 6, 2020
@adamwojs adamwojs deleted the ezp_31875 branch October 6, 2020 14:54
@micszo micszo removed their assignment Oct 12, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

Successfully merging this pull request may close these issues.

6 participants