Single-write handler and action suffix issue fix : #12988
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Description
We have a heavily event-driven solution built using PhalconPHP in-depth. Therefore, while the dispatch-loop is running, there are certain occasions when we naturally perform forwards, handle exceptions and so on.
We use multiple different controller action suffixes based on execution context (managed by undisclosed preconditions).
The problem we were facing was, that whenever one of our listeners were forwarding and modifying the suffix of the running dispatcher, the new suffix was not taken into consideration.
I have found in the source code, that the problem's root cause is, that the controller action suffix is read once, before the dispatch loop (the while cycle) kicks off. This can easily be fixed, and I do not expect unwanted side-effects based on the source code, common sense, and local testing.
Patched versions / release branches
Patch explanation
My aim with the changes was to do as little change as possible with little to no possibility for unexpected side-effects.
moved local value definition within while loop
On version 2.0.0 the move refactor seemed to be the least risky and smallest change as I could not find the expected getter for the Controller. The patch would have felt half-assed if I'd only fix the action part.
changed action method value assignment to getter instead of local string concatenation.
On all other versions I could see, that the controller suffix was always part of the loop. Therefore, I just had to change the action method definition from string concatenation to the getter. The getter is a better approach as it respects the object state.
From :
let actionMethod = actionName . actionSuffix;
To :
let actionMethod = this->getActiveMethod();
In raising this pull request, I confirm the following (please check boxes):
The given method has no proper test coverage, and to write up the proper mocking, preconditions, post-conditions and assertions to assure, that this trivial change works would consume too much of my free time. We are talking about a ~275ish line long method, with a cyclomatic complexity way over 12 (just by looking at it, not counting).