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

Need to check that response has not been already sent in phalcon/mvc/micro.zep #12668

Closed
wants to merge 9 commits into from

Conversation

Fabsolute
Copy link

@Fabsolute Fabsolute commented Mar 1, 2017

Hello!

  • Type: bug fix

In raising this pull request, I confirm the following :

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

Small description of change:

It is not possible to return a string value from a mapped function for an HTTP Method, and then catch that returned value by using Micro::getReturnedValue() method inside handler on Micro::after($handler), its overriding my response.

Thanks

It is not possible to return a string value from a mapped function for an HTTP Method, and then catch that returned value by using Micro::getReturnedValue() method inside handler on Micro::after($handler), its overriding my response.
@Jurigag
Copy link
Contributor

Jurigag commented Mar 1, 2017

This is bug fix. It should be made to 3.0.x, @sergeyklay well i read your comment in previous PR that it should be to 3.1.x, so okay, also update changelog. Also it could be good to provide test that what you mention is fixed.

@Fabsolute
Copy link
Author

Fabsolute commented Mar 3, 2017

Changelog updated. Since I could not install zephir on macox sierra, I could not tested.

@Jurigag
Copy link
Contributor

Jurigag commented Mar 8, 2017

I mean unit test.

Copy link
Member

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

@Fabsolute In general I like this PR. Could you please provide a small test which covers these changes?

Copy link
Member

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

@Fabsolute

FILE: /home/travis/build/phalcon/cphalcon/tests/unit/Mvc/MicroTest.php
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 451 | ERROR | [x] Expected 1 space after USE keyword; found 0
     |       |     (Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterUse)
 451 | ERROR | [x] Expected 1 space before opening brace; found 0
     |       |     (Squiz.Functions.MultiLineFunctionDeclaration.SpaceBeforeBrace)
 458 | ERROR | [x] Expected 1 space before opening brace; found 0
     |       |     (Squiz.Functions.MultiLineFunctionDeclaration.SpaceBeforeBrace)
 459 | ERROR | [x] Line indented incorrectly; expected at least 24
     |       |     spaces, found 23
     |       |     (Generic.WhiteSpace.ScopeIndent.Incorrect)
 467 | ERROR | [x] The closing brace for the class must go on the next
     |       |     line after the body
     |       |     (PSR2.Classes.ClassDeclaration.CloseBraceAfterBody)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

return "success";
}
);
$app->handle("/api");
Copy link
Member

Choose a reason for hiding this comment

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

expect($micro->handle('/api'))->equals("success"); //❓ 

@sergeyklay
Copy link
Member

Fixed in the 3.2.x branch. Thank you for contributing.

@sergeyklay sergeyklay closed this Apr 12, 2017
@Fabsolute Fabsolute deleted the 3.1.x branch June 6, 2017 18:24
@Fabsolute Fabsolute restored the 3.1.x branch June 6, 2017 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants