This caused regressions in Prophecy for code that worked fine on PHP 7.0 - there's no compelling reason for getType to return a string with ? when there is no ? in the source code.

there's no compelling reason for getType to return a string with ? when there is no ? in the source code.

Unfortunately we still have no way to know that - allowsNull is useless as it returns TRUE for both Foo $foo = NULL and ?Foo $foo. It still needs to be fixed somehow (new method isNullable?) before GA.

Given function foo(Foo $foo = null) {} is there an error for foo(null)? No. It allows null. This is why the type of $foo is ?Foo and allowsNull() is true.

As noted in the RFC, a function declaration that looks like this:

function foo(Foo $f = null);

Implies

function foo(?Foo $f = null);

This is important to understand because there is no semantic difference in the type whether or not it is ?Foo or Foo $f = null. They have the same type.

Whether it is correct to generate a ? or not is up to you based on the version of PHP you are targeting.

@morrisonleci It's logical but is a BC break.

Aside from that getType should return a type. ?Foo is not a type, it's a notation for a union.

@ciaranmcnulty: I think @morrisonlevi was answering to @Majkl578 saying that "allowsNull is useless as it returns TRUE for both Foo $foo = NULL and ?Foo $foo",
and I agree with @morrisonlevi here: there is absolutely no semantic difference between both notations, they are pure coding style with no technical implication for ReflectionType.

@Majkl578: ReflectionType is about reflecting a type hint. It allows one to know what type is accepted/returned here. And there can be only 2 answers to this: Foo or Foo or null. Which is what the getName() and allowsNull() methods allow you to get access to.

Just tried this against zend-code and ProxyManager, and it seems to work. @ciaranmcnulty @nicolas-grekas does this work against your test suites?

The only issue I had so far is that the deprecation notices cause test suites to fail. This is zendframework/zend-code:2.0.3, for example:

∙ ../../php-src/sapi/cli/php ./vendor/bin/phpunit
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

SSSSSSSS................................
Fatal error: Method ReflectionNamedType::__toString() must not throw an exception, caught PHPUnit_Framework_Error_Deprecated: Function ReflectionType::__toString() is deprecated in /Users/ocramius/Documents/Projects/zend/zend-code/src/Generator/ParameterGenerator.php on line 0

Not sure whether the E_DEPRECATED is a mess or not here. The simple solution for me was to simply add convertErrorsToExceptions="false" to phpunit.xml.dist, but there is still some level of breakage here, as you can see.

I'm still for the deprecation, just adding a question to the bucket. @nicolas-grekas fyi, when symfony/yaml deprecated the loading of files directly, we all hated the guts out of it for a few months :-\

You can set up your error reporting to ignore E_DEPRECATED or try using an error handler instead since ignoring all warnings and non-exception errors is usually undesirable. That's more code/configuration but it is probably better solution.

Yes, the only issue is that it's still existing code that may be broken. Most setups ignore deprecation errors in production environments.

I just threw in the problem: as I said, phpunit config fix did the trick for me.

@Ocramius if you want to be more selective about deprecations, you can set PHPUnit_Framework_Error_Deprecated::$enabled = false, or better: use Symfony's PHPUnit bridge (well, after this patch).

By default, I get the same fatal error... which is quite unfortunate: many userland error handlers out there (e.g. phpunit's) turn any errors into exceptions. Yet, throwing from __toString turns them into fatal errors :((((

Note also that phpunit uses ReflectionType->__toString() in its mock system so that the deprecation will be triggered quite often even if the tested code doesn't use the method.

In Symfony, to remain compatible with error handlers that turn anything into exceptions, all deprecation notices are silently triggered, that is: @trigger_error(..., E_USER_DEPRECATED);. This makes dealing with deprecations opt-in, ie when one doesn't care, it's invisible, but when one does, a custom error handler can still catch them (and that's what the phpunit bridge does).

I think that's a really wise triggering policy. It didn't come as an obvious one and we were skeptical at first. But 18 months since this decision, we know it's really the best choice to keep a strict BC policy, yet allow opt-inner to move forward with us.

Back to this thread, the deprecation is also a BC break, since it breaks our test suites. If the outcome weren't a fatal error, I'd be ok to say "hey, php core needs to trigger deprecations before dropping code in next major version".

But here and with all of this in mind, I'd prefer not deprecate the method... Let it be, it returns the same thing as getName(). Maybe not the best if we could rewind in time. But we can't :)

Ugh, this is a really unfortunate complication. So essentially, this means we effectively can't deprecate __toString() methods without first solving the issue of exceptions from __toString() :/

On the other hand, I feel like converting a deprecation notice to an exception pretty much explicitly opts you out of the BC policy. We reserve the right to introduce deprecations, but if you convert them to exceptions that definitely can't be backwards compatible for you anymore.

Of course, this is a somewhat idealistic view given how PHPUnit does this by default (and I can't say that this default behavior is unreasonable).

I guess I could live with just deprecating it in the documentation, along the lines of:

This method is deprecated as of PHP 7.1 and will be removed in PHP 8.0. Due to technical reasons, it does not throw a deprecation notice.

converting a deprecation notice to an exception pretty much explicitly opts you out of the BC policy

Yep, agreed. Because php core doesn't trigger that many deprecations, error handler author (phpunit included imho) didn't noticed deprecations need special care. Still php core should trigger these.

I could live with just deprecating it in the documentation

👍

I could live with just deprecating it in the documentation

@dshafik what's your take on this? Seems sensible in this scenario, even if it was never done before...

It's probably worth asking @sebastianbergmann whether this PHPUnit issue could be fixed, if 7.1 was throwing deprecation notices.

If you ask me, deprecation is the exact kind of thing a test suite SHOULD discover. It would be nice if PHPUnit issued a warning, rather than causing this fatal error however.

@sebastianbergmann thoughts on handling E_DEPRECATED specially? e.g. a section at the end of the output detailing deprecated usage. IIRC converting to exceptions is part of the flow control for handling PHP errors/warning in PHPUnit, so it might not be feasible.

I was hoping for @sebastianbergmann to chime in here, however I still feel that catching deprecations, even through failure, is part of the job of the test suite. We can add the deprecation at a later date, and just document it as such for now. Can you add a comment in the code to this effect @nikic?

I'd like to get this merged today so I can build RC3.

@dshafik Done

Are we ready to merge? /cc @nikic

I've merged the changes. I've left the un-deprecation as a separate commit, so that actually deprecating it just needs a git revert.

The issues in Prophecy are resolved in RC3