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

BC break in return type of ConstraintViolationInterface::getMessage() #34710

Closed
Majkl578 opened this issue Nov 29, 2019 · 12 comments
Labels

Comments

@Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Nov 29, 2019

Symfony version(s) affected: 4.4.0

Description
In Symfony 4.3.4 Symfony\Component\Validator\ConstraintViolationInterface::getMessage() returns string.
In Symfony 4.4.0 it returns string|object.

Widening return types is a BC break that breaks contract for consumers.

Caught by PHPStan:

Parameter #1 $message of class InvalidArgumentException constructor expects string, object|string given.
@ro0NL

This comment has been minimized.

Copy link
Contributor

@ro0NL ro0NL commented Nov 29, 2019

ref #31083, in general the upgrade path is (string) getMessage(). Agree the BC break is real, but only with strict_types involved, isnt it?

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

@Majkl578 Majkl578 commented Nov 29, 2019

@ro0NL No, converting arbitrary object (as specified in the phpDoc, equiv. to string|object union in PHP 8) into string is a fatal error without strict mode as well.

php > (string) new stdClass();
PHP Recoverable fatal error:  Object of class stdClass could not be converted to string in php shell code on line 1

#31083 is about __toString() but the type information doesn't match this requirement.

@xabbuh was right with his concern in #31083:

I am 👎 on making this change as it calls for breaking existing code that do not perform explicit string casts on the returned value of getMessage().

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 29, 2019

As mentioned by the phpdoc, not any kind of object can be returned there: only stringable ones are allowed. That was already the case before and people (eg Drupal) are using this possibility as a feature.

Not BC break here. That's a false-positive from phpstan (which might miss context for sure).

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

@Majkl578 Majkl578 commented Nov 29, 2019

Not BC break here. That's a false-positive from phpstan (which might miss context for sure).

Frankly by adding |object union you made it a BC break - there is no constaint for stringable objects on type level in PHP and a comment for @return annotation is not sufficient.

PHPStan is correct in this case. Just like Psalm is. We're talking about interface with generic string|object.

https://phpstan.org/r/14597979-9220-4ce1-8354-14e3954d44ca
https://psalm.dev/r/e2520d6222

@stof

This comment has been minimized.

Copy link
Member

@stof stof commented Nov 29, 2019

the thing is, phpdoc does not have any way to describe "stringable objects".

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

@Majkl578 Majkl578 commented Nov 29, 2019

@stof Yes, adding |object is considered a strong BC break but some kind of stringable type would be too because objects are not implicitly convertible to strings in strict mode:
https://3v4l.org/K5fFZ
https://3v4l.org/hTYEZ
https://3v4l.org/5gkJs

So this is wrong:

Not BC break here. That's a false-positive from phpstan (which might miss context for sure).

This has to be reverted.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 29, 2019

Our BC policy doesn't cover strict mode. Thanks for reporting.

@Majkl578

This comment has been minimized.

Copy link
Contributor Author

@Majkl578 Majkl578 commented Nov 29, 2019

Reading through Symfony Backward Compatibility Promise document I see no mention of not covering strict mode. Strict mode is part of PHP core for 4 years already, it would be strange if Symfony didn't support it (regardless of whether Symfony itself uses it or not). Can you provide some reference for your statement?
Thank you.

All interfaces shipped with Symfony can be used in type hints. You can also call any of the methods that they declare. We guarantee that we won't break code that sticks to these rules. ... If you implement an interface, we promise that we won't ever break your code.

@enumag

This comment has been minimized.

Copy link
Contributor

@enumag enumag commented Nov 30, 2019

Our BC policy doesn't cover strict mode. Thanks for reporting.

@nicolas-grekas Considering most of modern PHP projects (all of my projects included) are written completely in strict mode, this is a very concerning statement. Even more so since there is no mention of this in your BC promise document. Please update it accordingly since this is something the users need to be aware of.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 30, 2019

Strict mode is not compatible with adding type hints in the Symfony code base. Any string $foo added anywhere is a candidate break for ppl that enabled strict mode and pass anything that PHP can cast transparently to a string. There is nothing we can do here about this, except never adding any type hints in the code base, which is not what you're asking for I suppose. Quite the contrary actually. So here it is: strict mode is a bad idea, and you know why now.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 30, 2019

To clarify, the link to the BC policy here is not accurate: what I'm referring to is the continuous upgrade path actually - we cannot provide one to people that enabled strict mode, for cases like the one mentioned in my previous comment.

About the BC policy and the issue discussed here, it does not apply to docblocks. That's the more accurate statement that applies here.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 2, 2019

Last note: the LazyString class proposed in #34298 is designed to fix the inaccuracy of string|object: instead, we'll be able to use string|LazyString and get type safety.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.