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

Compatibility for ReflectionFunctionAbstract and ReflectionFunction #40

Merged
merged 14 commits into from
Jul 8, 2015

Conversation

asgrim
Copy link
Member

@asgrim asgrim commented Jul 5, 2015

Depends on #32 being merged first, possibly rebase after etc.

This introduces compatibility for #7 items ReflectionFunction and ReflectionFunctionAbstract where possible. Some items are not implement, but marked accordingly in the compatibility list (with corresponding GitHub issues)

@asgrim asgrim force-pushed the compat-reflectionfunctionabstract branch from 5b0656f to a224087 Compare July 7, 2015 08:54
@asgrim asgrim changed the title [WIP] Compatibility for ReflectionFunctionAbstract and ReflectionFunction Compatibility for ReflectionFunctionAbstract and ReflectionFunction Jul 7, 2015
@asgrim asgrim assigned Ocramius and unassigned asgrim Jul 7, 2015
@@ -50,10 +53,12 @@ public function getName()
public function isMatchingReflector(Reflection $reflector)
{
if ($this->name == self::IDENTIFIER_CLASS) {
return $reflector instanceof ReflectionClass;
return ($reflector instanceof ReflectionClass);
Copy link
Member

Choose a reason for hiding this comment

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

parenthesis not needed

@asgrim
Copy link
Member Author

asgrim commented Jul 7, 2015

@Ocramius all done

if (isset($stmt->stmts) && is_array($stmt->stmts) && count($stmt->stmts)) {
if ($this->checkStatementsForYield($stmt->stmts)) {
return true;
}
Copy link

Choose a reason for hiding this comment

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

Yield is an expression, it does not necessarily occur as a statement. So you should probably do a full traversal to find usages.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nikic ahh nice catch there, thanks for pointing this out

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks again @nikic - I hope you don't mind but I borrowed your test cases to check we've covered all the various yields (in PHP5 at least), I've credited you here, let me know if you object or anything!

@asgrim asgrim mentioned this pull request Jul 7, 2015
1 task
@@ -53,7 +56,9 @@ public function isMatchingReflector(Reflection $reflector)
return $reflector instanceof ReflectionClass;
}

// @todo add more type checks
Copy link
Member

Choose a reason for hiding this comment

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

constants?

Copy link
Member Author

Choose a reason for hiding this comment

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

one day

Copy link
Member

Choose a reason for hiding this comment

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

one issue

Copy link
Member Author

Choose a reason for hiding this comment

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

#48

Ocramius added a commit that referenced this pull request Jul 8, 2015
Compatibility for ReflectionFunctionAbstract and ReflectionFunction
@Ocramius Ocramius merged commit 46b3d0e into master Jul 8, 2015
@Ocramius Ocramius deleted the compat-reflectionfunctionabstract branch July 8, 2015 07:36
@Ocramius
Copy link
Member

Ocramius commented Jul 8, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants