Convert more zlib warnings to exceptions #12708
Conversation
This PR further extends the functionality of a previous PR #4985. Ran into this while working on a [particular WordPress functionality](Automattic/jetpack#34186) - our servers reported tons of warnings. Currently, the only way to avoid warnings is to call `@gzinflate`, but silencing errors is discouraged so we also have to `phpcs:ignore` it. If `gzinflate` threw an exception instead, it would be much easier for developers to handle this. Applied same reasoning to all the zlib functions. I also found out that the `json` extension also does not use any notices: ``` bor0:~/dev/php-src/ext/json$ grep -R php_error_docref * bor0:~/dev/php-src/ext/json$ ```⚠️ This is a breaking change and may cause code flow interruptions unless handled with exceptions⚠️
|
The implicit policy is that we don't use exceptions/errors for states that cannot be programmatically checked in advance. As such, I don't think we can do this. The BC break especially feels like this requires an RFC. |
|
@Girgias, thanks for your comment.
So if we strictly follow this policy (which isn't necessarily followed for other extensions across), does that mean silencing function calls is the preferred way to do it properly?
I am okay to file an RFC for this, maybe after I get more comments about what people think of this PR in general. Just a note, #4985 created similar backwards incompatibility - curious if it had (or if it was necessary to have) an RFC? |
The errors introduced in that PR are things that you can check in advance are always valid, such as the compression argument being in a valid range or that one uses one of the ZLIB constants, i.e. programming errors. The warnings you are trying to promote are warnings that are emitted by the C library, and cannot be checked in advance as they depend on the archive. |
This PR further extends the functionality of a previous PR #4985.
Ran into this while working on a particular WordPress functionality - our servers reported tons of warnings. Currently, the only way to avoid warnings is to call
@gzinflate, but silencing errors is discouraged so we also have tophpcs:ignoreit. Ifgzinflatethrew an exception instead, it would be much easier for developers to handle this.Applied same reasoning to all the zlib functions.
I also found out that the
jsonextension also does not use any notices: