Skip to content

Compatibility for ReflectionParameter#9

Merged
Ocramius merged 17 commits intomasterfrom
reflection-parameter-compat
Jul 2, 2015
Merged

Compatibility for ReflectionParameter#9
Ocramius merged 17 commits intomasterfrom
reflection-parameter-compat

Conversation

@asgrim
Copy link
Copy Markdown
Member

@asgrim asgrim commented Jul 1, 2015

As part of #7, this PR brings full compatibility with ReflectionParameter (where possible)

  • isArray
  • isCallable
  • getPosition
  • isDefaultValueAvailable
  • canBePassedByValue
  • getClass
  • getDefaultValueConstantName
  • isDefaultValueConstant
  • isPassedByReference
  • isVariadic

@asgrim asgrim added this to the 1.0.0 milestone Jul 2, 2015
@asgrim asgrim assigned Ocramius and asgrim and unassigned Ocramius Jul 2, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If there is a composite type, do you return Type[] or CompositeType?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As per discussion, this returns the type hint defined in the language

@asgrim asgrim changed the title [WIP] Compatibility for ReflectionParameter Compatibility for ReflectionParameter Jul 2, 2015
@asgrim asgrim assigned Ocramius and unassigned asgrim Jul 2, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the problem with recursion here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We'd have to load a ReflectionClass instance of the class that is hinted in the language type hint, which could be the same class. Currently, because we are not autoloading classes, and the PR #3 is still WIP, we have no way of really loading a ReflectionClass for this, unless we used builtin ReflectionClass, but that kinda defeats the point. It's a mess that I need to have a think on...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@asgrim so... registry of parsed classes (so far)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

private function doLoadClass($class, & $currentlyLoaded = [])
{
    if (isset($currentlyLoaded[$class])) {
        return $currentlyLoaded[$class];
    }

    // ... possible recursion here.
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've created new issue #13 to investigate this - for now I'm happy to say it's currently unsupported and implement later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Split into a separate private method?

Ocramius added a commit that referenced this pull request Jul 2, 2015
Compatibility for ReflectionParameter
@Ocramius Ocramius merged commit 6e1b0c0 into master Jul 2, 2015
@Ocramius Ocramius deleted the reflection-parameter-compat branch July 2, 2015 10:36
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.

2 participants