thecodingmachine / safe

All PHP functions, rewritten to throw exceptions instead of returning false
MIT License
2.36k stars 142 forks source link

ob_end_clean signature isn't quite right, IMO #224

Open jaydiablo opened 4 years ago

jaydiablo commented 4 years ago

Ran into some code that was doing this:

while (@ob_end_clean()) {}

Safe defines ob_end_clean as a void return: https://github.com/thecodingmachine/safe/blob/master/generated/outcontrol.php#L22

However, ob_end_clean will return true when it successfully clears the buffer.

php > ob_start();
php > ob_start();
php > ob_start();
php > ob_start();
php > $i = 0;
php > $out = [];
php > while ($result = ob_end_clean()) { $out[$i] = $result; $i++; }
PHP Notice:  ob_end_clean(): failed to delete buffer. No buffer to delete in php shell code on line 1
php > var_dump($out);
array(4) {
  [0]=>
  bool(true)
  [1]=>
  bool(true)
  [2]=>
  bool(true)
  [3]=>
  bool(true)
}

If we use Safe\ob_end_clean in this situation, static analyzers complain that the output of a void returning function is used (since it's used in the while loop's condition).

Shouldn't the signature of this function remain as returning a bool (just true if the false condition throws the Safe exception)?

We could modify the code to do this:

try {
    while (true) {
        ob_end_clean();
    }
} catch (Safe\Exceptions\OutcontrolException $e) {
}

Which is fine, just doesn't seem to match what users of the native ob_end_clean function may be expecting.

asherkin commented 4 years ago

How about:

while (ob_get_level() > 0) {
    ob_end_clean();
}

I think that would end up the same result, be static analysis safe, and actually throw in case of a non-cleanable buffer?

Kharhamel commented 4 years ago

@jaydiablo Do you agree with @asherkin or do you still think we should do a special case for ob_end_clean?