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

Convert HTTP method to uppercase #2712

Closed
wants to merge 68 commits into from

Conversation

Projects
None yet
@lyrixx
Copy link
Contributor

lyrixx commented Apr 7, 2019

Q A
Bug fix? no, but prevent bug in client application
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

pborreli and others added some commits Feb 2, 2019

Merge pull request #2289 from Toflar/use-security-expression-language
Use framework expression language service instead of custom one
Merge pull request #2510 from dunglas/fix_2492
GraphQL: Prevent an error when the type factory is null
Merge pull request #2531 from soyuka/merge
merging 2.4 into master nothing to see here
Enable the pagination via cursor (no page-based pagination)
Add tests for the SoMany collection

Uses the PropertyAccessor service and don't break the BC

Allow to use multiple fields

Fix cs

Improve cursor-based pagination tests
Merge pull request #2532 from soyuka/sroze-cursor-pagination
Enable the pagination via cursor (no page-based pagination)
Merge pull request #2557 from soyuka/merge
Merge 2.4 onto master
[GraphQl] Better pagination support (#2142)
* Better support for graphql pagination

Add support for graphql before, last, startcursor

* Allow limit 0 for MongoDB

* Fix backwards pagination

* Add hasPreviousPage
Merge pull request #2567 from norkunas/formats-provider-alias
Configure formats provider interface autowiring alias
[GraphQL] Avoid to call serialize in ItemNormalizer (#2576)
* Avoid to call serialize in ItemNormalizer

* Allow composite identifiers in getItemIriFromResourceClass

soyuka and others added some commits Apr 6, 2019

@lyrixx lyrixx force-pushed the lyrixx:ensure-verbe-is-uppercase branch from 182cdab to b219add Apr 7, 2019

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 7, 2019

Could you add a test?

Show resolved Hide resolved composer.json Outdated
Show resolved Hide resolved src/Metadata/Resource/Factory/OperationResourceMetadataFactory.php Outdated
@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 8, 2019

Can't we just convert the HTTP method to uppercase, since that's what they should be in anyway?

@lyrixx

This comment has been minimized.

Copy link
Contributor Author

lyrixx commented Apr 8, 2019

Can't we just convert the HTTP method to uppercase, since that's what they should be in anyway?

If you think this is a good approach, let's do it. I will update the PR

@lyrixx lyrixx force-pushed the lyrixx:ensure-verbe-is-uppercase branch from 0a21e6b to 28369bb Apr 8, 2019

@lyrixx lyrixx changed the title Added a protecting when someone is using a lower case method Convert HTTP method to uppercase Apr 8, 2019

@lyrixx lyrixx force-pushed the lyrixx:ensure-verbe-is-uppercase branch 2 times, most recently from 47b2e43 to 843e491 Apr 8, 2019

@lyrixx

This comment has been minimized.

Copy link
Contributor Author

lyrixx commented Apr 8, 2019

PR Updated

@@ -123,6 +123,8 @@ private function normalize(bool $collection, ResourceMetadata $resourceMetadata,
$supported ? $operation['method'] = $upperOperationName : $operation['route_name'] = $operationName;
}
$operation['method'] = strtoupper($operation['method']);

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Apr 8, 2019

Member

This does not seem correct to me. $operation['method'] might not be set.

Anyway, this part of the code could probably use some refactoring. $upperOperationName is... weird. 🙈

@lyrixx lyrixx force-pushed the lyrixx:ensure-verbe-is-uppercase branch from 843e491 to 637e4c2 Apr 8, 2019

@teohhanhui teohhanhui changed the base branch from master to 2.4 Apr 15, 2019

@teohhanhui teohhanhui closed this Apr 15, 2019

@teohhanhui

This comment has been minimized.

Copy link
Member

teohhanhui commented Apr 15, 2019

I'll merge it in #2734 as @lyrixx does not allow pushing to his branch. 🙈

@lyrixx lyrixx deleted the lyrixx:ensure-verbe-is-uppercase branch Apr 16, 2019

@lyrixx

This comment has been minimized.

Copy link
Contributor Author

lyrixx commented Apr 16, 2019

I'll merge it in #2734 as @lyrixx does not allow pushing to his branch.

Oups, sorry. This was not intended

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.