Improved ReflectionType API so that a target reflection class can directly be retrieved, removed existing named constructor
#415
Conversation
…e found, removed existing named constructor
| use RuntimeException; | ||
| use function sprintf; |
There was a problem hiding this comment.
Is it really needed?
Tests passes without this.
In the RFC, there is this text: While it's true that you can import a namespace and alias it to a single character, this is not necessary for classes, so it makes no sense to require it for functions.
There was a problem hiding this comment.
Gonna likely propose the adoption of doctrine/coding-standard here, which enforces all functions to be imported to avoid static analysis issues, as well as runtime function lookups.
There was a problem hiding this comment.
Hmm, just seen that there already is an open pull request for that.
There was a problem hiding this comment.
Ok thanks :)
It's interesting point of view.
For to avoid static analysis issues is it not a problem from static analysis?
And for function lookups I don't understand where is the problem with that.
Have you an example? To help me to understand where it could be a problem.
There was a problem hiding this comment.
if you call a function foo() you have no idea if it's a function declared in the same namespace or a global function. PHP falls back on the global scope for functions and constants whereas you never have the ambiguity with classes: you either need to use the FQCN e.g. \Closure or import the symbol with a use statement use Closure
There was a problem hiding this comment.
For
to avoid static analysis issuesis it not a problem from static analysis?
an example I have is this one:
namespace Potato;
define('POTATO', 'POTATO');Is Potato::POTATO defined here? Or is define() a user-defined function?
You can only be sure of it if it was written this way:
namespace Potato;
use function define;
define('POTATO', 'POTATO');I hope that clears it up.
UPGRADE.md
Outdated
| @@ -4,6 +4,13 @@ This document serves as a reference to upgrade your current | |||
| BetterReflection installation if improvements, deprecations | |||
| or backwards compatibility (BC) breakages occur. | |||
|
|
|||
| ## 4.0.0 | |||
There was a problem hiding this comment.
Is there a reason why not to make it part of version 3?
There was a problem hiding this comment.
I just thought 3.0.0 was already released: did this in a real rush, so will amend 👍
…hey now contain a whole reflector
Problem: I run XDebug locally, and XDebug ovverloads `var_dump()`, so the output is wildly different on travis-ci.
|
Apart from whatever is causing failed build, looks ok at a glance |
|
@asgrim all green here 👍 |
Ocramius
changed the title
ReflectionType API so that a target reflection class can be found, removed existing named constructorReflectionType API so that a target reflection class can directly be retrieved, removed existing named constructor
Required for Roave/BackwardCompatibilityCheck#38