-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix ReflectionType::__toString() BC break #2136
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
Fix ReflectionType::__toString() BC break #2136
Conversation
@@ -3031,7 +3031,7 @@ ZEND_METHOD(reflection_type, __toString) | |||
|
|||
str = reflection_type_name(param); | |||
|
|||
if (param->arg_info->allow_null) { | |||
if (param->arg_info->allow_null & 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you define a constant instead of using a magic number here ?
Anyway, in 7.1 , you'll need to change your code because of the nullable feature being introduced.
|
You will need to change the code for new features, but old code should still reflect the same. That's what BC is all about, and I'm kinda sick of explaining it, at this point. Development of zendframework/zend-code, doctrine/common and ocramius/proxy-manager halted again :-\ |
@Ocramius If we keep the BC then zendframework/zend-code, doctrine/common and ocramius/proxy-manager get to keep working but it's basically useless for everyone else. That's what BC breaks are all about and I'm kinda sick of explaining it, at this point. |
still need to see that "everyone else". We discussed it, and it ended up with "it's internal stuff that you can't haz". |
@Ocramius So you don't respect NDAs or other forms of respecting material that aren't yours? |
I agree, still, breaking few million projects (dependants) vs breaking your Marco Pivetta On Wed, Sep 21, 2016 at 8:52 PM, Levi Morrison notifications@github.com
|
If you are writing code generators I hardly think this is the most difficult BC break. In fact, I'm fairly certain you've spent more time fighting the change than it would have been to just make it compatible. You already know what to do if you are on 7.0; if you are on 7.1 just take a different code path with the new API. Seriously, I offer right now to fix it for you as long as you'll collaborate with me on it (since I'm obviously not as familiar with the projects as you are). |
I actually worked an entire week to introduce compat with I already fixed this shit, the point is that this shit shouldn't need Oh, and I'm dropping PHP 7.0 support due to the complexity of maintaining Marco Pivetta On Wed, Sep 21, 2016 at 9:01 PM, Levi Morrison notifications@github.com
|
👍 for this - Prophecy is impacted for basically no reason. We obviously have work to do to support Nullable types but breaking |
Closing in favor of #2137 |
PHP 7.1 has a BC break that is hitting us hard, that is:
ReflectionType::__toString()
now adds a?
in front of type hints wherenull
is allowed by their default values.But since
zend_bool
is really anunsigned char
, we have plenty of room to store and forward this semantic subtlety (nullable being set through?Foo $arg
vsFoo $arg = null
) so thatReflectionType::__toString()
can be accurate again, thus BC.