Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:46886
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 83.243.58.133 as permitted sender)
To: Matt Read <matt@mattread.com>
Cc: internals@lists.php.net, Pierrick Charron <pierrick@php.net>, Ilia Alshanetsky <iliaa@php.net>
In-Reply-To: <1263606897.3850.27.camel@elisa>
References: <1263606897.3850.27.camel@elisa>
Content-Type: text/plain; charset="UTF-8"
Organization: php.net
Date: Tue, 26 Jan 2010 10:11:16 +0100
Message-ID: <1264497076.3201.74.camel@guybrush>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] PDO Bug introduced in revision 290786 and released
 in 5.2.12 and 5.3.1
From: johannes@php.net (Johannes =?ISO-8859-1?Q?Schl=FCter?=)

On Fri, 2010-01-15 at 20:54 -0500, Matt Read wrote:
> We are developers from the Habari Project, an open source PHP blogging
> application; We would like to raise concern with a recent change to the
> logic of PDO.
> 
> We believe that PHP's revision 290786 [1] should not have been committed
> (see bug 49521 [2]). This changed the behaviour of PDO so that an
> object's constructor is called before the properties from the database
> fields are set.

Any more comments on this one? - The more I think Matt is right on this
one especially as the PROPS_LATE constant exists in this case.

We have two similar cases:

in regards to tidy:
http://svn.php.net/viewvc?view=revision&revision=292620
and in regards to mysqli:
http://svn.php.net/viewvc?view=revision&revision=293039

A problem with these changes is that the old behavior made it possible
to do further object initialization once the data had been provided by
using a constructor. With the new code the user has to add an "init"
method of some kind which is called after fetching the data as
object ...

The old behavior might be wrong from an OO-puristic point of view, but
that isn't fixed, actually as the properties are still randomly set.

I think best is to revert these 3 changes from 5.2, 5.3 and trunk and
make it clearer in the docs.

johannes

> The behaviour requested in bug 49521 [2] was possible already with
> PDO_FETCH_PROPS_LATE, and indeed this commit renders that flag
> completely useless. You should also note that this flag is still
> checked, for no apparent reason, in the code.
>  
> Several PHP tickets about the same issue have been closed as bogus
> before this one was committed. See 43717 [3] and 37134 [4], especially
> the closing comment on the latter, "This is expected, to allow people to
> access the data from the query within the [constructor] ..."
> 
> Unfortunately this commit has now made into two releases, 5.2.12 and
> 5.3.1, and has broken our software. We relied on the behaviour (the
> expected behaviour) to have properties set (using magic __set) before
> the constructor was called, to determine what data came from the DB, and
> what data has been updated since.
> 
> Revision 290786 seems to have been committed with insufficient thought
> put in to what was being done, especially given that it was committed
> only 4 days before release. Now we don't have a choice in how PDO
> constructs objects, as we did before with the FETCH_PROPS_LATE flag.
> There were even tests checking that the constructor was called last, the
> tests were changed rather than questioning whether the existing
> behaviour was correct. Not to mention a significant behavioural change
> to PDO being pushed into a minor release, again, 4 days before release.
> 
> Part of the problem is that it doesn't seem well documented (if at all)
> that setting the properties before calling the constructor is the
> intended and correct behaviour. Or rather, was.
> 
> [1] http://svn.php.net/viewvc/?view=revision&revision=290786
> [2] http://bugs.php.net/bug.php?id=49521
> [3] http://bugs.php.net/bug.php?id=43717
> [4] http://bugs.php.net/bug.php?id=37134
> 
> 
> Thanks kindly,
> 
> Matt Read
> Michael Harris
> Richard Cockrum
> Luke Giuliani
> 
> 
> 
>