From f7c0bca88dd9082ce5474ccc11ee7f4e36a93072 Mon Sep 17 00:00:00 2001 From: soyuka Date: Tue, 13 Sep 2022 20:17:32 +0200 Subject: [PATCH] style: various code smells --- features/graphql/authorization.feature | 2 +- features/main/custom_operation.feature | 2 +- src/Action/ExceptionAction.php | 7 ++++--- src/Api/IdentifiersExtractor.php | 3 ++- .../QueryParameterValidator/QueryParameterValidator.php | 6 ++++-- src/Doctrine/Common/State/LinksHandlerTrait.php | 6 +++--- src/GraphQl/Action/EntrypointAction.php | 2 +- src/GraphQl/Serializer/SerializerContextBuilder.php | 2 +- .../Factory/SerializerPropertyMetadataFactory.php | 4 ++-- src/Serializer/AbstractItemNormalizer.php | 2 +- src/Symfony/Routing/ApiLoader.php | 6 ++---- .../TestBundle/Entity/SubresourceOrganization.php | 8 +++----- .../Resolver/UploadMultipleMediaObjectResolver.php | 2 -- 13 files changed, 25 insertions(+), 27 deletions(-) diff --git a/features/graphql/authorization.feature b/features/graphql/authorization.feature index a9c16a7131b..dd5ad290a62 100644 --- a/features/graphql/authorization.feature +++ b/features/graphql/authorization.feature @@ -410,7 +410,7 @@ Feature: Authorization checking """ Then the response status code should be 200 And the response should be in JSON - And the JSON node "data.securedDummy.ownerOnlyProperty" should be equal to the string "" + And the JSON node "data.securedDummy.ownerOnlyProperty" should be equal to "" Scenario: A user cannot retrieve an item they doesn't own When I add "Authorization" header equal to "Basic ZHVuZ2xhczprZXZpbg==" diff --git a/features/main/custom_operation.feature b/features/main/custom_operation.feature index 9fa829ec5c7..bcea29860cc 100644 --- a/features/main/custom_operation.feature +++ b/features/main/custom_operation.feature @@ -96,7 +96,7 @@ Feature: Custom operation @createSchema Scenario: Void a payment - Given There is a payment + Given there is a payment When I send a "POST" request to "/payments/1/void" Then the response status code should be 201 And the response should be in JSON diff --git a/src/Action/ExceptionAction.php b/src/Action/ExceptionAction.php index 02373922de2..d6901b89cd4 100644 --- a/src/Action/ExceptionAction.php +++ b/src/Action/ExceptionAction.php @@ -85,12 +85,13 @@ private function getOperationExceptionToStatus(Request $request): array $resourceMetadataCollection = $this->resourceMetadataCollectionFactory->create($attributes['resource_class']); /** @var HttpOperation $operation */ $operation = $resourceMetadataCollection->getOperation($attributes['operation_name'] ?? null); - $exceptionToStatus = $operation->getExceptionToStatus() ?: []; + $exceptionToStatus = [$operation->getExceptionToStatus() ?: []]; + foreach ($resourceMetadataCollection as $resourceMetadata) { /** @var ApiResource $resourceMetadata */ - $exceptionToStatus = array_merge($exceptionToStatus, $resourceMetadata->getExceptionToStatus() ?: []); + $exceptionToStatus[] = $resourceMetadata->getExceptionToStatus() ?: []; } - return $exceptionToStatus; + return array_merge(...$exceptionToStatus); } } diff --git a/src/Api/IdentifiersExtractor.php b/src/Api/IdentifiersExtractor.php index 16eba0ecc5c..f77095de9aa 100644 --- a/src/Api/IdentifiersExtractor.php +++ b/src/Api/IdentifiersExtractor.php @@ -75,7 +75,8 @@ public function getIdentifiersFromItem(object $item, Operation $operation = null continue; } - $identifiers[$link->getParameterName()] = $this->getIdentifierValue($item, $link->getFromClass(), $link->getIdentifiers()[0], $link->getParameterName()); + $parameterName = $link->getParameterName(); + $identifiers[$parameterName] = $this->getIdentifierValue($item, $link->getFromClass(), $link->getIdentifiers()[0], $parameterName); } return $identifiers; diff --git a/src/Api/QueryParameterValidator/QueryParameterValidator.php b/src/Api/QueryParameterValidator/QueryParameterValidator.php index b56861ab515..1a36f6e6284 100644 --- a/src/Api/QueryParameterValidator/QueryParameterValidator.php +++ b/src/Api/QueryParameterValidator/QueryParameterValidator.php @@ -61,13 +61,15 @@ public function validateFilters(string $resourceClass, array $resourceFilters, a foreach ($filter->getDescription($resourceClass) as $name => $data) { foreach ($this->validators as $validator) { - $errorList = array_merge($errorList, $validator->validate($name, $data, $queryParameters)); + if ($errors = $validator->validate($name, $data, $queryParameters)) { + $errorList[] = $errors; + } } } } if ($errorList) { - throw new FilterValidationException($errorList); + throw new FilterValidationException(array_merge(...$errorList)); } } } diff --git a/src/Doctrine/Common/State/LinksHandlerTrait.php b/src/Doctrine/Common/State/LinksHandlerTrait.php index ccddbc4fa0f..badd9d3df50 100644 --- a/src/Doctrine/Common/State/LinksHandlerTrait.php +++ b/src/Doctrine/Common/State/LinksHandlerTrait.php @@ -53,9 +53,9 @@ private function getLinks(string $resourceClass, Operation $operation, array $co // Instead, we'll look for the first Query available. foreach ($resourceMetadataCollection as $resourceMetadata) { - foreach ($resourceMetadata->getGraphQlOperations() as $operation) { - if ($operation instanceof Query) { - $linkedOperation = $operation; + foreach ($resourceMetadata->getGraphQlOperations() as $op) { + if ($op instanceof Query) { + $linkedOperation = $op; } } } diff --git a/src/GraphQl/Action/EntrypointAction.php b/src/GraphQl/Action/EntrypointAction.php index e9bbbfdd75a..f4c9af93e68 100644 --- a/src/GraphQl/Action/EntrypointAction.php +++ b/src/GraphQl/Action/EntrypointAction.php @@ -156,7 +156,7 @@ private function applyMapToVariables(array $map, array $variables, array $files) throw new BadRequestHttpException('GraphQL multipart request file has not been sent correctly.'); } - foreach ($map[$key] as $mapValue) { + foreach ($value as $mapValue) { $path = explode('.', (string) $mapValue); if ('variables' !== $path[0]) { diff --git a/src/GraphQl/Serializer/SerializerContextBuilder.php b/src/GraphQl/Serializer/SerializerContextBuilder.php index e24ade98515..e4a32779a25 100644 --- a/src/GraphQl/Serializer/SerializerContextBuilder.php +++ b/src/GraphQl/Serializer/SerializerContextBuilder.php @@ -88,7 +88,7 @@ private function replaceIdKeys(array $fields, ?string $resourceClass, array $con continue; } - $denormalizedFields[$this->denormalizePropertyName((string) $key, $resourceClass, $context)] = \is_array($fields[$key]) ? $this->replaceIdKeys($fields[$key], $resourceClass, $context) : $value; + $denormalizedFields[$this->denormalizePropertyName((string) $key, $resourceClass, $context)] = \is_array($value) ? $this->replaceIdKeys($value, $resourceClass, $context) : $value; } return $denormalizedFields; diff --git a/src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php b/src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php index cab79a776dc..3c1bbf0e630 100644 --- a/src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php +++ b/src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php @@ -198,9 +198,9 @@ private function getClassSerializerGroups(string $class): array $groups = []; foreach ($serializerClassMetadata->getAttributesMetadata() as $serializerAttributeMetadata) { - $groups = array_merge($groups, $serializerAttributeMetadata->getGroups()); + $groups[] = $serializerAttributeMetadata->getGroups(); } - return array_unique($groups); + return array_unique(array_merge(...$groups)); } } diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php index a8b95f7cb8b..66d15269fd9 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -263,7 +263,7 @@ protected function instantiateObject(array &$data, string $class, array &$contex throw new RuntimeException(sprintf('Cannot create an instance of %s from serialized data because the variadic parameter %s can only accept an array.', $class, $constructorParameter->name)); } - $params = array_merge($params, $data[$paramName]); + $params[] = $data[$paramName]; } } elseif ($allowed && !$ignored && (isset($data[$key]) || \array_key_exists($key, $data))) { $params[] = $this->createConstructorArgument($data[$key], $key, $constructorParameter, $context, $format); diff --git a/src/Symfony/Routing/ApiLoader.php b/src/Symfony/Routing/ApiLoader.php index ceeee5350c0..923a9b50110 100644 --- a/src/Symfony/Routing/ApiLoader.php +++ b/src/Symfony/Routing/ApiLoader.php @@ -75,10 +75,8 @@ public function load(mixed $data, string $type = null): RouteCollection $path = str_replace(sprintf('{%s}', $parameterName), $expandedValue, $path); } - if ($controller = $operation->getController()) { - if (!$this->container->has($controller)) { - throw new RuntimeException(sprintf('There is no builtin action for the "%s" operation. You need to define the controller yourself.', $operationName)); - } + if (($controller = $operation->getController()) && !$this->container->has($controller)) { + throw new RuntimeException(sprintf('There is no builtin action for the "%s" operation. You need to define the controller yourself.', $operationName)); } $route = new Route( diff --git a/tests/Fixtures/TestBundle/Entity/SubresourceOrganization.php b/tests/Fixtures/TestBundle/Entity/SubresourceOrganization.php index 560a96a5519..20ed9de5836 100644 --- a/tests/Fixtures/TestBundle/Entity/SubresourceOrganization.php +++ b/tests/Fixtures/TestBundle/Entity/SubresourceOrganization.php @@ -75,11 +75,9 @@ public function addSubresourceEmployee(SubresourceEmployee $employee): self public function removeSubresourceEmployee(SubresourceEmployee $employee): self { - if ($this->employees->removeElement($employee)) { - // set the owning side to null (unless already changed) - if ($employee->getSubresourceOrganization() === $this) { - $employee->setSubresourceOrganization(null); - } + // set the owning side to null (unless already changed) + if ($this->employees->removeElement($employee) && $employee->getSubresourceOrganization() === $this) { + $employee->setSubresourceOrganization(null); } return $this; diff --git a/tests/Fixtures/TestBundle/GraphQl/Resolver/UploadMultipleMediaObjectResolver.php b/tests/Fixtures/TestBundle/GraphQl/Resolver/UploadMultipleMediaObjectResolver.php index 61cf6270dd6..06dae7ee6e6 100644 --- a/tests/Fixtures/TestBundle/GraphQl/Resolver/UploadMultipleMediaObjectResolver.php +++ b/tests/Fixtures/TestBundle/GraphQl/Resolver/UploadMultipleMediaObjectResolver.php @@ -26,7 +26,6 @@ class UploadMultipleMediaObjectResolver implements MutationResolverInterface { public function __invoke(?object $item, array $context): MediaObject { - $result = []; $mediaObject = null; /** @@ -40,7 +39,6 @@ public function __invoke(?object $item, array $context): MediaObject $mediaObject = new MediaObject(); $mediaObject->id = $key; $mediaObject->contentUrl = $uploadedFile->getFilename(); - $result[] = $mediaObject; } // Currently API Platform does not support custom mutation with collections so for now, we are returning the last created media object.