Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not compare + segments unless wildcards in either accept or priority #94

Merged
merged 2 commits into from
May 14, 2017

Conversation

weierophinney
Copy link
Contributor

Fixes #93. If there are no wildcard segments in either the subtype or + segment of either the Accept header or priority, do not attempt to compare them.

Adds more tests for #92 as well, using * subtypes in both the Accept header and priorities, to validate that different combinations work as expected.

Fixes willdurand#93. If there are no wildcard segments in either the subtype or +
segment of either the Accept header or priority, do not attempt to
compare them.

Adds more tests for willdurand#92 as well, using `*` subtypes in both the Accept
header and priorities, to validate that different combinations work as
expected.
@willdurand willdurand self-requested a review May 8, 2017 16:17
@weierophinney
Copy link
Contributor Author

@willdurand , @meyerbaptiste — ready to review!

@weierophinney
Copy link
Contributor Author

And @dunglas !

@dunglas
Copy link
Contributor

dunglas commented May 8, 2017

Our test suite is green with this patch (see api-platform/core#1104), thanks @weierophinney!

@@ -49,11 +49,17 @@ protected function match(AcceptHeader $accept, AcceptHeader $priority, $index)
list($acceptSub, $acceptPlus) = $this->splitSubPart($acceptSub);
list($prioritySub, $priorityPlus) = $this->splitSubPart($prioritySub);

// If no wildcards in either the subtype or + segment, do nothing.
if (! ($acceptBase === '*' || $baseEqual)
|| ! ($acceptSub === '*' || $prioritySub === '*' || $acceptPlus === '*' || $priorityPlus === '*')) {
Copy link

Choose a reason for hiding this comment

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

According to PSR-2 when the if is multi-line the ) { needs to be on it's own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprised my linter didn't catch that! Fixing now and pushing momentarily. Also, for internal consistency, removing spaces following ! operators.

- Move closing paren/open brace to following line
- No space following negation operator
@dunglas
Copy link
Contributor

dunglas commented May 10, 2017

@willdurand do you think you'll soon have time soon to merge this patch and create a new release? It's currently breaking all API Platform projects when running composer update but we can prevent this by tagging a release on our side preventing to install 2.3+.

@willdurand
Copy link
Owner

@willdurand do you think you'll soon have time soon to merge this patch and create a new release? It's currently breaking all API Platform projects when running composer update but we can prevent this by tagging a release on our side preventing to install 2.3+.

doing this right now, was on holidays earlier this week..

@willdurand willdurand merged commit 03436ed into willdurand:2.x May 14, 2017
@willdurand
Copy link
Owner

willdurand commented May 14, 2017

@dunglas just tagged 2.3.1

@willdurand
Copy link
Owner

Thanks everyone! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants