cedaro / satispress

Expose installed WordPress plugins and themes as Composer packages.
500 stars 49 forks source link

Don't use silence error operator #72

Closed GaryJones closed 5 years ago

GaryJones commented 5 years ago

After https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1450, the @ silenced errors are being reported as violations.

For example, in the HTTP/ResponseBody/FileBody.php file:

  72 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @ob_flush()... (WordPress.PHP.NoSilencedErrors.Discouraged)
  87 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @session_write_close()... (WordPress.PHP.NoSilencedErrors.Discouraged)
  90 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @apache_setenv( 'no-gzip',... (WordPress.PHP.NoSilencedErrors.Discouraged)
  95 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @set_magic_quotes_runtime( 0 ... (WordPress.PHP.NoSilencedErrors.Discouraged)
  99 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @ini_set( 'zlib.output_compression',... (WordPress.PHP.NoSilencedErrors.Discouraged)
 100 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @set_time_limit( 0 ... (WordPress.PHP.NoSilencedErrors.Discouraged)
 101 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @ob_end_clean()... (WordPress.PHP.NoSilencedErrors.Discouraged)
 103 | WARNING | Silencing errors is strongly discouraged. Use proper error checking instead. Found: @ob_end_clean()... (WordPress.PHP.NoSilencedErrors.Discouraged)

The simple solution would be to change the // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged to // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged.

However, I've looked at the documentation for some of the above, and they don't say anything about triggering an error on failure, or if not supported. We could wrap some of them in function_exists(), or try/catch statements, or otherwise try and handle the possibility of an error being thrown in a different way if there's no better solution. With us now supporting PHP 7+, the set_magic_quotes_runtime() shouldn't even be needed.


There is also a use of @filesize() in another file. The new sniff allows for a default list of silenced PHP functions to not be reported. The default, including for WordPress-Core is that it is supported, but WordPress-Extra disables this. It can be re-enabled with:

    <rule ref="WordPress.PHP.NoSilencedErrors">
        <properties>
            <property name="use_default_whitelist" value="true"/>
        </properties>
    </rule>
bradyvercher commented 5 years ago

I'm pretty sure the only place the silence operators are used is in relation to the download code, which is similar to the approach used in WooCommerce and EDD, although I suspect it probably originated from somewhere else. I don't know if those are used based on experience or just an abundance of caution.

The documentation doesn't say anything about some of them triggering errors, but there's evidence that they may do so in some environments. For instance, here's a commit for set_time_limit() in WooCommerce where the appropriate checks are already in place, but a warning was still triggered.

Since errors can be caught in PHP 7+, I think we can wrap that method in a try/catch block and drop the silence operators.

filesize() seems to be used throughout core without the silence operator and if it does generate an error, it might be worth knowing about.

bradyvercher commented 5 years ago

I'm not sure 7e84720 will actually catch notices, so that might need some more testing.

bradyvercher commented 5 years ago

I haven't seen any errors when testing or deploying these changes in production, so I'm going to close it out. Getting more people using this in more environments should let us know if there's anything else we need to do.