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

Single-write handler and action suffix issue fix : #12988

Closed
wants to merge 1 commit into from

Conversation

adam-rocska
Copy link

Issue Description

  • Type: bug fix

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

Version Patch applied
2.0.0 moved local value definition within while loop
2.0.x changed action method value assignment to getter instead of local string concatenation.
2.1.x changed action method value assignment to getter instead of local string concatenation.
3.0.x changed action method value assignment to getter instead of local string concatenation.
3.1.x changed action method value assignment to getter instead of local string concatenation.
3.2.x changed action method value assignment to getter instead of local string concatenation.
4.0.x changed action method value assignment to getter instead of local string concatenation.

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):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

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).

- local actionMethod variable value is now set to be the return value of getActiveMethod(); which reads from properties, respecting the Dispatcher's state in execution time.

@sergeyklay
Copy link
Contributor

See #12993 (comment)

@sergeyklay
Copy link
Contributor

Closing in favor of #13004. Thank you for contributing!

@sergeyklay sergeyklay closed this Aug 3, 2017
@niden niden added bug A bug report status: low Low and removed Bug - Low labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report status: low Low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants