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.

Sign up for GitHub

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

TASK: Avoid accessing "global" request, response and arguments in action controller #4

Closed
wants to merge 1 commit into from
Closed

TASK: Avoid accessing "global" request, response and arguments in action controller #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions Neos.Flow/Classes/Mvc/Controller/AbstractController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,23 +110,25 @@ abstract class AbstractController implements ControllerInterface
*/
protected function initializeController(ActionRequest $request, ActionResponse $response)
{
// make the current request "globally" available to everywhere in this controller.
$this->request = $request;
$this->request->setDispatched(true);
$this->response = $response;

$this->request->setDispatched(true);

$this->uriBuilder = new UriBuilder();
$this->uriBuilder->setRequest($this->request);
$this->uriBuilder->setRequest($request);

$this->arguments = new Arguments([]);
$this->controllerContext = new ControllerContext($this->request, $this->response, $this->arguments, $this->uriBuilder);
$this->controllerContext = new ControllerContext($request, $response, $this->arguments, $this->uriBuilder);

$mediaType = MediaTypeHelper::negotiateMediaType(MediaTypeHelper::determineAcceptedMediaTypes($request->getHttpRequest()), $this->supportedMediaTypes);
if ($mediaType === null) {
$this->throwStatus(406);
}
$this->negotiatedMediaType = $mediaType;
if ($request->getFormat() === '') {
$this->request->setFormat(MediaTypes::getFilenameExtensionFromMediaType($mediaType));
$request->setFormat(MediaTypes::getFilenameExtensionFromMediaType($mediaType));
}
}

Expand Down Expand Up @@ -370,10 +372,10 @@ protected function throwStatus(int $statusCode, $statusMessage = null, $content
* @throws \Neos\Flow\Security\Exception
* @api
*/
protected function mapRequestArgumentsToControllerArguments(ActionRequest $request)
protected function mapRequestArgumentsToControllerArguments(ActionRequest $request, Arguments $arguments)
{
/* @var $argument \Neos\Flow\Mvc\Controller\Argument */
foreach ($this->arguments as $argument) {
foreach ($arguments as $argument) {
$argumentName = $argument->getName();
if ($argument->getMapRequestBody()) {
$argument->setValue($request->getHttpRequest()->getParsedBody());
Expand Down
38 changes: 19 additions & 19 deletions Neos.Flow/Classes/Mvc/Controller/ActionController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,9 @@ public function processRequest(ActionRequest $request): ActionResponse

$this->actionMethodName = $this->resolveActionMethodName($request);

$this->initializeActionMethodArguments();
$this->initializeActionMethodArguments($this->arguments);
if ($this->enableDynamicTypeValidation !== true) {
$this->initializeActionMethodValidators();
$this->initializeActionMethodValidators($this->arguments);
}

$this->initializeAction();
Expand All @@ -241,14 +241,14 @@ public function processRequest(ActionRequest $request): ActionResponse
}

try {
$this->mapRequestArgumentsToControllerArguments($request);
$this->mapRequestArgumentsToControllerArguments($request, $this->arguments);
} catch (RequiredArgumentMissingException $e) {
$message = $this->throwableStorage->logThrowable($e);
$this->logger->notice('Request argument mapping failed due to a missing required argument. ' . $message, LogEnvironment::fromMethodName(__METHOD__));
$this->throwStatus(400, null, 'Required argument is missing');
}
if ($this->enableDynamicTypeValidation === true) {
$this->initializeActionMethodValidators();
$this->initializeActionMethodValidators($this->arguments);
}

if ($this->view === null) {
Expand All @@ -260,8 +260,7 @@ public function processRequest(ActionRequest $request): ActionResponse
$this->initializeView($this->view);
}

// We still use a global response here as it might have been changed in any of the steps above
Copy link
Author

Choose a reason for hiding this comment

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

if someone dared to replace $this->response above - not just mutate the object!!! - i dont know what i should think.

$response = $this->callActionMethod($request, $this->response);
$response = $this->callActionMethod($request, $this->arguments, $response);

if (!$response->hasContentType()) {
$response->setContentType($this->negotiatedMediaType);
Expand Down Expand Up @@ -301,7 +300,7 @@ protected function resolveActionMethodName(ActionRequest $request): string
* @throws InvalidArgumentTypeException
* @see initializeArguments()
*/
protected function initializeActionMethodArguments()
protected function initializeActionMethodArguments(Arguments $arguments)
{
$actionMethodParameters = static::getActionMethodParameters($this->objectManager);
if (isset($actionMethodParameters[$this->actionMethodName])) {
Expand All @@ -310,7 +309,7 @@ protected function initializeActionMethodArguments()
$methodParameters = [];
}

$this->arguments->removeAll();
$arguments->removeAll();
foreach ($methodParameters as $parameterName => $parameterInfo) {
$dataType = null;
if (isset($parameterInfo['type'])) {
Expand All @@ -326,7 +325,7 @@ protected function initializeActionMethodArguments()
$dataType = TypeHandling::stripNullableType($dataType);
}
$mapRequestBody = isset($parameterInfo['mapRequestBody']) && $parameterInfo['mapRequestBody'] === true;
$this->arguments->addNewArgument($parameterName, $dataType, ($parameterInfo['optional'] === false), $defaultValue, $mapRequestBody);
$arguments->addNewArgument($parameterName, $dataType, ($parameterInfo['optional'] === false), $defaultValue, $mapRequestBody);
}
}

Expand Down Expand Up @@ -390,7 +389,7 @@ protected function getInformationNeededForInitializeActionMethodValidators()
*
* @return void
*/
protected function initializeActionMethodValidators()
protected function initializeActionMethodValidators(Arguments $arguments)
{
[$validateGroupAnnotations, $actionMethodParameters, $actionValidateAnnotations, $actionIgnoredArguments] = $this->getInformationNeededForInitializeActionMethodValidators();

Expand Down Expand Up @@ -420,7 +419,7 @@ protected function initializeActionMethodValidators()
}

/* @var $argument Argument */
foreach ($this->arguments as $argument) {
foreach ($arguments as $argument) {
$argumentName = $argument->getName();
if (isset($ignoredArguments[$argumentName]) && !$ignoredArguments[$argumentName]['evaluate']) {
continue;
Expand Down Expand Up @@ -515,17 +514,18 @@ protected function initializeAction()
* view exists, the view is rendered automatically.
*
* @param ActionRequest $request
* @param ActionResponse $response
* @return ActionResponse
* @param Arguments $arguments
* @param ActionResponse $response The most likely empty response to modify or replace.
* @return ActionResponse The final response for this request.
*/
protected function callActionMethod(ActionRequest $request, ActionResponse $response): ActionResponse
protected function callActionMethod(ActionRequest $request, Arguments $arguments, ActionResponse $response): ActionResponse
{
$preparedArguments = [];
foreach ($this->arguments as $argument) {
foreach ($arguments as $argument) {
$preparedArguments[] = $argument->getValue();
}

$validationResult = $this->arguments->getValidationResults();
$validationResult = $arguments->getValidationResults();

if (!$validationResult->hasErrors()) {
$actionResult = $this->{$this->actionMethodName}(...$preparedArguments);
Expand Down Expand Up @@ -560,12 +560,12 @@ protected function callActionMethod(ActionRequest $request, ActionResponse $resp
}

if ($actionResult === null && $this->view instanceof ViewInterface) {
$this->response = $this->renderView($this->response);
$response = $this->renderView($response);
Copy link
Author

Choose a reason for hiding this comment

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

this assignment on $this->response was a thorn in my eyes

} else {
$this->response->setContent($actionResult);
$response->setContent($actionResult);
}

return $this->response;
return $response;
}

/**
Expand Down
6 changes: 2 additions & 4 deletions Neos.Flow/Tests/Unit/Mvc/Controller/AbstractControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,12 +505,11 @@ public function mapRequestArgumentsToControllerArgumentsDoesJustThat()
}

$controller = $this->getAccessibleMock(AbstractController::class, ['processRequest']);
$controller->_set('arguments', $controllerArguments);

$this->mockActionRequest->expects(self::atLeast(2))->method('hasArgument')->withConsecutive(['foo'], ['baz'])->willReturn(true);
$this->mockActionRequest->expects(self::atLeast(2))->method('getArgument')->withConsecutive(['foo'], ['baz'])->willReturnOnConsecutiveCalls('bar', 'quux');

$controller->_call('mapRequestArgumentsToControllerArguments', $this->mockActionRequest);
$controller->_call('mapRequestArgumentsToControllerArguments', $this->mockActionRequest, $controllerArguments);
self::assertEquals('bar', $controllerArguments['foo']->getValue());
self::assertEquals('quux', $controllerArguments['baz']->getValue());
}
Expand All @@ -533,11 +532,10 @@ public function mapRequestArgumentsToControllerArgumentsThrowsExceptionIfRequire
}

$controller = $this->getAccessibleMock(AbstractController::class, ['processRequest']);
$controller->_set('arguments', $controllerArguments);

$this->mockActionRequest->expects(self::exactly(2))->method('hasArgument')->withConsecutive(['foo'], ['baz'])->willReturnOnConsecutiveCalls(true, false);
$this->mockActionRequest->expects(self::once())->method('getArgument')->with('foo')->willReturn('bar');

$controller->_call('mapRequestArgumentsToControllerArguments', $this->mockActionRequest);
$controller->_call('mapRequestArgumentsToControllerArguments', $this->mockActionRequest, $controllerArguments);
}
}
3 changes: 1 addition & 2 deletions Neos.Flow/Tests/Unit/Mvc/Controller/ActionControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,6 @@ public function initializeActionMethodValidatorsDoesNotAddValidatorForIgnoredArg
$this->actionController->expects(self::any())->method('getInformationNeededForInitializeActionMethodValidators')->will(self::returnValue([[], [], [], $ignoredValidationArguments]));

$this->inject($this->actionController, 'actionMethodName', 'showAction');
$this->inject($this->actionController, 'arguments', $arguments);

$this->inject($this->actionController, 'objectManager', $this->mockObjectManager);

Expand All @@ -423,6 +422,6 @@ public function initializeActionMethodValidatorsDoesNotAddValidatorForIgnoredArg
$mockArgument->expects(self::never())->method('setValidator');
}

$this->actionController->_call('initializeActionMethodValidators');
$this->actionController->_call('initializeActionMethodValidators', $arguments);
}
}
Loading