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.
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
Hi.
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.
I fully support what Matt writes. Although we don't use that behaviour
in our codebase, the way this change was done raises concerns on our
side as well.
Especially the timeframe and the handling of the existing tests make me
worry in that context.
@Matt: What entry in the bug tracker is related to this new issue?
Regards,
Karsten
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=37134Thanks kindly,
Matt Read
Michael Harris
Richard Cockrum
Luke Giuliani
I agree that for the PDO and mysqli, if I put my OO-puristic side
away, it can make some sense to call the constructor after
initialization of the object. I agree that those changes can be
reverted but this really have to be well documented.
About the Tidy one (Bug #50558), there I'm not sure... I think the new
behaviour is the right one. If you extend the tidy object it make
sense (in my opinion) to allow usage of the type hinting and functions
like is_a. I probably missed something, can you provide me an example
of why it make no sense to leave this change ?
Regards.
Pierrick
2010/1/26 Johannes Schlüter johannes@php.net:
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=293039A 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=37134Thanks kindly,
Matt Read
Michael Harris
Richard Cockrum
Luke Giuliani
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=293039A 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.
Yup, I agree.
Derick
--
http://derickrethans.nl | http://xdebug.org
twitter: @derickr