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

Jump to bottom

Fix static callable. #2054

Merged
merged 1 commit into from
Aug 18, 2016
Merged

Fix static callable. #2054

merged 1 commit into from
Aug 18, 2016

Conversation

SpacePossum
Copy link
Contributor

fixes #2053

@fabpot
Copy link
Contributor

fabpot commented Jun 3, 2016

Can you add some tests to avoid any regressions?

@SpacePossum SpacePossum force-pushed the 1_x_bug_2053 branch 2 times, most recently from c8124f7 to fa2ae48 Compare June 3, 2016 12:23
@SpacePossum
Copy link
Contributor Author

I've added the tests that walk through all possible paths of the changed code.
I reverted a smaller change I did earlier as it was not needed.

fabpot
fabpot reviewed Jun 21, 2016
View reviewed changes
lib/Twig/Node/Expression/Call.php
if ($r instanceof ReflectionMethod) {
$callableName = $r->class.'::'.$r->name;
} else {
$callableName = $r->name;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved back where it was before. This was done on purpose so that the $callableName is always defined (for HHVM/Hack).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if I follow, $callableName will always be defined here. Is there something special with HHVM/Hack related to the previous $r->getDeclaringClass() or something?

Copy link
Member

Choose a reason for hiding this comment

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

@fabpot it is always defined in the new logic

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, there is no need to change this part of the code.

Copy link
Contributor Author

@SpacePossum SpacePossum Jun 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

yes there is, if $r is not a ReflectionMethod calling getDeclaringClass on it will result in the error reported
(tests to prove this have been added)

@SpacePossum
Copy link
Contributor Author

ping @fabpot I think this one ready, please let me know if you anything changed :)

@fabpot
Copy link
Contributor

fabpot commented Aug 18, 2016

Thank you @SpacePossum.

@fabpot fabpot merged commit 8f6fd57 into twigphp:1.x Aug 18, 2016
fabpot added a commit that referenced this pull request Aug 18, 2016
@fabpot
bug #2054 Fix static callable. (SpacePossum)
d726bde 
This PR was merged into the 1.x branch.

Discussion
----------

Fix static callable.

fixes #2053

Commits
-------

8f6fd57 Fix exception message for static method and object without req. params.
@SpacePossum SpacePossum deleted the 1_x_bug_2053 branch August 18, 2016 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SpacePossum @fabpot @stof