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

strict_types Breaks Consuming Code #36

Closed
tjlytle opened this issue Feb 20, 2020 · 2 comments
Closed

strict_types Breaks Consuming Code #36

tjlytle opened this issue Feb 20, 2020 · 2 comments
Labels
Invalid This doesn't seem right

Comments

@tjlytle
Copy link

tjlytle commented Feb 20, 2020

Note: This could be totally intentional, and it was for a major version, so not a problem. But wanted to make sure this was the intention. I don't think it was as the change log doesn't mention it as a breaking change, or even reference the potential issue:

zendframework/zend-diactoros#329 adds return type hints and scalar parameter type hints wherever possible. The changes were done to help improve code quality, in part by reducing manual type checking. If you are extending any classes, you may need to update your signatures; check the signatures of the class(es) you are extending for changes.

BC Break Report

Q A
Version 2.x.x

Summary

Because strict_types are enabled in the library, but method arguments lack type hints (due to the interface understandably not type hinting arguments), Stream::write($string) and other methods accept values that can be juggled into the expected type (and were in the past), but now are rejected by the underlying f*() methods with a TypeError:

Previous behavior

$stream->write(false); // would write ''

Current behavior

$stream->write(false); // throws TypeError
TypeError : fwrite() expects parameter 2 to be string, bool given

How to reproduce

See my branch for a test / fix specific to write(); however, there are many other methods that have the same behavior.

We fixed on our end, I just wanted to make sure this behavior was known / documented.

@Xerkus Xerkus added the Invalid This doesn't seem right label Feb 20, 2020
@Xerkus
Copy link
Member

Xerkus commented Feb 20, 2020

https://github.com/php-fig/http-message/blob/efd67d1dc14a7ef4fc4e518e7dee91c271d524e4/src/StreamInterface.php#L108-L115

    /**
     * Write data to the stream.
     *
     * @param string $string The string that is to be written.
     * @return int Returns the number of bytes written to the stream.
     * @throws \RuntimeException on failure.
     */
    public function write($string);

As per the method signature false is not a valid value and it never was before.
Introduction of strict types did not change the signature so it is not a BC break for supported usage. In effect, what you experience is a bug on consumer side that was silently swallowed before but now it got exposed.

Aside for some specific cases, we implicitly trust framework users to adhere to interface as introduction of checks for all possible misuses will have negative impact on code readability and performance. There are tools that can be used as part of QA to detect those problems. Psalm and phpstan are probably the most used open source tools.

Generally, while I understand where you come from with a sloppy PHP type juggling usage custom in the community, depending on undefined and unsupported behavior have severe impact on code reliability and maintainability as highlighted here. I would recommend to proactively detect and fix it.

Since I do not consider this a bug on our side, I will mark the issue as invalid.

@Xerkus Xerkus closed this as completed Feb 20, 2020
@tjlytle
Copy link
Author

tjlytle commented Feb 21, 2020

@Xerkus not sure I'd call it a consumer side bug, as before enabling strict types, type juggling could be reasonably expected (and to some extent, is still ambiguous, because the type is no more than annotation, not a proper hint).

But I don't disagree with closing this at all. Thanks for the quick reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants