Bug #97707
open
Fluid forms return old values
0%
Description
TYPO3 unserializes old values and sends them to controller action.
Steps to reproduce;
1) Setup an action that simply returns a default response.
- Should be blank so to speak.
- Should have one parameter. For simplicity, make it be a DTO with one
parameter for the input field and a reference a simple validator.
2) Setup the validator. Make it simple, something like if string is longer than 3 characters (or whatever) and return an error if shorter.
3) Setup a simple form with one input field for the validator and a submit button.
4) After setup, run the following test.
- Submit a VALID string. The form will return with no errors.
- Then, submit an INVALID string. It will be accepted.
I know. You are like, what?? Yeah. It will be accepted, even though the validator said it has errors.
Where things go wrong:
Here: \TYPO3\CMS\Extbase\Mvc\Controller\ActionController::forwardToReferringRequest() . In this method, arguments are sought from __referrer internal arguments instead of the submitted values. Normally, if errors are found, only two elements are found in the arguments form variable: controller for controller name and action for action name. This is because they were submitted originally with the form and they will recycle as long as the validator finds errors. Nothing else will be added to it. This is good... until it validates. When it validates, results are sent directly to the action controller and not through the error controller. When the process goes back to the form, the object or arguments submitted will be added to the form and returned to the user. Remember it validated. When you then send a wrong value, it goes to the error controller, old values (that validated) are unserialized and forwarded to your action. And that's how you end up with old values in your action.
Assumption:
Form post values processing seem to be built under the presumption that once a form validates, you won't need it again. This needs correcting.
Updated by John Miller almost 2 years ago
- Category changed from Extbase to Fluid
Just saw this in the doc section of \TYPO3\CMS\Fluid\ViewHelpers\FormViewHelper::renderHiddenReferrerFields() where values are serialized before being added to the form and returned to the user:
/**
* Renders hidden form fields for referrer information about
* the current controller and action.
*
* @return string Hidden fields with referrer information
* @todo filter out referrer information that is equal to the target (e.g. same packageKey)
*/
Please see the @todo part. Seems a solution has been planned but not yet implemented. Also, the arguments to be serialized do not contain packageKey . But it contains the action name that holds the forwarded (old) values.
A solution that works perfectly:
In the \TYPO3\CMS\Fluid\ViewHelpers\FormViewHelper::renderHiddenReferrerFields() method, make the following slight change;
protected function renderHiddenReferrerFields()
{
...
$actionName=$request->getControllerActionName();
$arguments=$request->getArguments();
if(isset($argument['controller'])){
$argument=[
'controller'=>$controllerName,
'action'=>$actionName
];
}
$actionRequest=[
'@extension'=>$extensionName,
'@controller'=>$controllerName,
'@action'=>$actionName,
];
...
$result.='<input type="hidden" name="'.htmlspecialchars($this->prefixFieldName('__referrer[arguments]')).'" value="'.htmlspecialchars($this->hashService->appendHmac(base64_encode(serialize($arguments)))).'" />'.LF;
...
return $result;
}
Updated by Torben Hansen almost 2 years ago
- Status changed from New to Needs Feedback
- Target version deleted (
next-patchlevel)
I was able to reproduce the described scenario and created this https://github.com/derhansen/validation_test little demo extension which covers the described steps to reproduce.
The main problem is, that the form uses the same action (to show the form and to process submitted data). By doing so, referring request data will always remain and although the DTO actually is not valid in the described 2nd form submission, the (invalid) data is still processed by the action (since this is the action to actually represent/show the validation error).
I highly recommend, that you at least have 2 different actions in your extension (e.g. newAction and createAction). The newAction is "blank" as you described and the createAction actually handles the processed data of the DTO and finally redirects the user back to newAction.
The Extbase documentation (see https://docs.typo3.org/m/typo3/book-extbasefluid/main/en-us/7-Controllers/1-Creating-Controllers-and-Actions.html#flow-pattern-creating-a-new-domain-object) describes the required flow in more detail.
Updated by Simon Schaufelberger almost 2 years ago
John, please use the pre tags to format your code. There is also a toolbar button for it ;)
Updated by John Miller almost 2 years ago
Thank you Torben for going through the trouble. Thank was quite some effort and time on your part. Again, wow, thank you. I wanted to come in and suggest to initialize the object. Your example does that. Also, for some reason I had forgotten completely about the two-action procedure.
Updated by John Miller almost 2 years ago
On a similar note, could TYPO3 readily initialize empty objects rather than passing null ? It would be great if a blank object, created via ReflectionClass without constructor initialization could be passed to the action.
$classReflection=new ReflectionClass(RequiredUninitializedDTO);
$argument = $classReflection->newInstanceWithoutConstructor();
Updated by John Miller almost 2 years ago
So I went digging deeper into the code and found a definitive explanation (or so I believe), in \TYPO3\CMS\Extbase\Mvc\Controller\ActionController::forwardToReferringRequest() method. This method is called by the error action. Error action is called when there is an error in validation. As I explained earlier, this method unwraps the __referrer[arguments] variable from the just submitted form and in it, contains your previous form values that passed validation earlier, and not the new (just submitted) values. These unwrapped values (only) are then forwarded to your action, which will readily be accepted without errors (because, remember, they already passed validation earlier, so they are good) instead of forwarding the new (just submitted) values. What this indicates is that form re-use after data values pass validation was never considered.
ADDITIONAL RELATED ISSUE (1):
Validation should be skipped after forwarding from error action, since validation is what brought us there in the first place. It doesn't make any sense to not check whether validation result already exist and skip validation if it does after forwarding.
ADDITIONAL RELATED ISSUE (2):
See \TYPO3\CMS\Extbase\Mvc\Controller\ActionController::processRequest() method. $this->renderAssetsForRequest($request); ought to be skipped when forwarding actions, and only be processed by final (non-forward) actions. Error action shouldn't process it.
Updated by Riccardo De Contardi over 1 year ago
- Status changed from Needs Feedback to New