php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #33910 Fix of bug #30578 not necessarily correct
Submitted: 2005-07-28 18:59 UTC Modified: 2005-07-29 10:37 UTC
From: jrweir at gmail dot com Assigned:
Status: Not a bug Package: Output Control
PHP Version: 5.1.0b3 OS: all
Private report: No CVE-ID: None
 [2005-07-28 18:59 UTC] jrweir at gmail dot com
Description:
------------
http://bugs.php.net/bug.php?id=30578
I don't think the way this bug was solved was correct. While I agree the example code posted above should work as expected, a side effect of fixing this caused all destructors to run before registered ob handlers. Destructors used to run after an ob handler, which was very useful for analyzing script output and setting flags for objects to clean themselves up appropriately. Consider the following (which is now not possible in 5.1):

<?php
ob_start('outputHandler');

function outputHandler($buffer){
    // find out if there was a fatal error. If none, allow sql connections to commit on destruct
    if (!preg_match('/((Fatal|Parse) error: .+ in .+ on line \d+)/', strip_tags($buffer), $matches)) SQL::setAllowCommit(true);
    return $buffer;
}


class SQL {

    private $connection;
    private static $allowCommit=false;

    public function __construct(){
    }

    public function __destruct(){
        if ($this->connection && self::getAllowCommit()) {
            $this->connection->commit();
            echo ' commit happened ';
        }
        else echo ' no commit happened ';
    }

    public static function getAllowCommit()     { return self::$allowCommit; }
    public static function setAllowCommit($val) { self::$allowCommit = $val; }

    public function connectToDatabase($host, $user, $pass, $db){
        $this->connection = new mysqli($host, $user, $pass, $db) or die('no connection');
        $this->connection->autocommit(false);
    }

    public function query($sql) {
        return $this->connection->query($sql);
    }
}


$dbOne = new SQL();
$dbOne->connectToDatabase('localhost', 'user', 'pass', 'db1');
$result = $dbOne->query("update tab set col = 'val'");

$dbTwo = new SQL();
$dbTwo->connectToDatabase('localhost', 'user', 'pass', 'db2');
$result = $dbTwo->query("update tab set col = 'val'");
?>

This simple example would be able to tell if a fatal error happened in the script and only allow the db connections to commit their transactions if there was no error. There is no way to do this now in php 5.1 (I realize I could achieve a similar affect by storing all the open connections and calling some other method from the ob handler, but then the destructors have no real purpose if not for cleanup based on the exit status of a script). The original example of this "bug" is really just pointing out contradicting functionality. I would assume that output buffering would not be allowed in destructors (as it is not allowed in output buffering handlers). If by waiting for all variables to go out of scope before the buffer flush, then there is even a bigger contradiction.




Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2005-07-28 19:13 UTC] tony2001@php.net
This is a chicken-and-egg problem that can't be solved in an easy way.
Please see bug #33772 for additional details.
If you have any ideas what should be the *right* order of calls - please tell your opinion. But remember that we have a lot of different handlers that are called on shutdown. 
See the current order in php-src/main/main.c, php_request_shutdown() function.
 [2005-07-28 19:41 UTC] jrweir at gmail dot com
Currently in 5.1.0b3 the order is
1. call destructors
2. call shutdown functions
3. flush output buffers

I think it should be
1. flush output buffers
2. call destructors
3. call shutdown functions

Reason:
Since calling ob_end*() in a destructor has never worked nobody has code written expecting it to. However, many people (as in bug #33772) have code written expecting destructors to run after output buffering. I'd say there are just as many valid reasons to flush buffers after the destructors run as there are to flush buffers before they run, but since it was the latter way since php 5, why change it?

Solution:
Make it illegal to do output buffering in a destructor just as it is in an output buffering handler.
 [2005-07-28 21:54 UTC] tony2001@php.net
>Since calling ob_end*() in a destructor has never worked
>nobody has code written expecting it to.

This is obviously not true and you know why: see bug #30578.
 [2005-07-28 23:07 UTC] jrweir at gmail dot com
Exactly, it had never worked until he requested it to work. That was an example of code that did not work, but does now because the shutdown order was changed (which in turn broke a bunch of people's code that was running under the assumption of a different shutdown order).

Before bug #30578:
- 0 people had working code that assumed ob flush ran after destruct because it didn't (one guy wanted it to though, so he reported it as a bug - but the bug solution should have been to disallow ob in destructors since they run after the flush in php 5.0)
- Many people had code that assumed ob flush ran before destruct

After bug #30578:
- 0 people have code that assumes ob flush ran after destruct (because it never did before, except for the guy that requested the "bug fix" because he just wanted the behaviour changed for his convienence. The php developer didn't think about the side effects it would cause when they changed the order.)
- Many people now have code that doesn't work because they understood the shutdown order and leveraged it to their advantage.


It seems to me that when you go stable with 5.1 and the shutdown order is switched, all the people who upgrade from 5.0 that didn't give 5.1b a test are going to be in for a suprise if their code depends on the old shutdown order.
 [2005-07-29 10:37 UTC] sniper@php.net
Actually this has nothing to do with bug #33772.
And the current shutdown order is the right one, it was broken before.

 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Apr 19 17:01:30 2024 UTC