Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

fputcsv() flagged when writing to stream #404

Closed paulschreiber closed 3 years ago

paulschreiber commented 5 years ago

Bug Description

The below code creates a CSV for download. PHPCS flags this as an error. I'm writing to a stream, not the filesystem.

Minimal Code Snippet

$output_stream = fopen( 'php://output', 'w' );

foreach ( $list as $row ) {
    fputcsv( $output_stream, $row );
}

Error Code

  88 | ERROR   | Filesystem writes are forbidden, you should not be using
     |         | fputcsv() (WordPress.VIP.FileSystemWritesDisallow)

Environment

Question Answer
PHP version 7.1.23
PHP_CodeSniffer version 3.4.1
VIPCS version 0.2.5
GaryJones commented 5 years ago

VIPCS version | 0.2.5

Does this still happen with VIPCS 0.4.0 (latest published version)?

How about master?

paulschreiber commented 5 years ago
GaryJones commented 5 years ago

How about master?

paulschreiber commented 5 years ago

@GaryJones Is there a specific commit or PR you think addresses this or a related problem? Or are you just enjoying having me randomly regress things?

Yes, the problem still occurs with master (97fa945).

GaryJones commented 5 years ago

Thank you.

Background notes:

rebeccahum commented 3 years ago

Due to the nature of static analysis, I think that that example would still flagged it because of $output_stream being a variable.

@jrfnl What are your thoughts for this one?

jrfnl commented 3 years ago

@rebeccahum I agree.

Just based on the call to fputcsv(), there is no way to know whether the output will be written to a file or a stream.

With the code snippet above, the sniff could walk back to find how $output_stream is declared and analyse that, but in reality, most code isn't this clean cut and the $handle ($output_stream variable) may be passed into a function doing the actual writes, as a function parameter in which case there is no way to know what was passed in.

We know it will be a resource handle, but that's it. Presuming it is a resource handle to php://output would be a huge jump and would leave any code which does actually attempt to write to a file undetected, which I think is a worse outcome.