Use early fetch and document why
c960657 - September 19, 2008 - 20:44
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | mfer |
| Status: | closed |
Description
After #298309: DatabaseStatement::execute with PDO::FETCH_CLASS calls __construct() after setting object attributes was fixed, I get one failed test when running the tests Database > Fetch tests.
Message: There is only one record.
Group: Other
Filename: database_test.test
Line: 188
Function: DatabaseFetchTestCase->testQueryFetchClass()Instead of returning 1 row, db_query() returns no rows at all.
I am using PHP 5.2.0-8+etch11 (Debian) with PDO Driver for MySQL client library version 5.0.32.
#1
I just ran the database tests and there were no errors. This was under PHP 5.2.5. I'm guessing there's a PHP bug that was fixed between 5.2.0 and 5.2.5 that covers this but I couldn't find it.
#2
It may be related to this bug that was fixed in PHP 5.2.4: http://bugs.php.net/bug.php?id=41971
With the workaround mentioned in the bug report the test passes:
function testQueryFetchClass() {$records = array();
$result = db_query("SELECT name FROM {test} WHERE age = :age", array(':age' => 25), array('fetch' => 'FakeRecord'));
- foreach ($result as $record) {
+ while ($record = $result->fetch(PDO::FETCH_CLASS | PDO::FETCH_PROPS_LATE)) {
$records[] = $record;
if ($this->assertTrue($record instanceof FakeRecord, t('Record is an object of class FakeRecord.'))) {
$this->assertIdentical($record->name, 'John', t('25 year old is John.'));
}
#3
It looks like the feature doesn't even work in the CVS version of 5.3.0, unless you call
$result->fetch(PDO::FETCH_CLASS | PDO::FETCH_PROPS_LATE). Calling$result->fetch()(without parameters) or iterating over rows usingforeach($result ...)does not make the constructor be called early.I just filed this bug for this: http://bugs.php.net/bug.php?id=46139 Whether it is expected behaviour or in fact a bug is hard to say, when there is no documentation.
#4
@c960657 it looks like you don't need the $statement->setFetchMode() part. If you remove that but pass in PDO::FETCH_CLASS | PDO::FETCH_PROPS_LATE, 'myClass' to your fetch or fetchAll method it works (This is how I have been doing it with PDO).
This looks like a bug with using setFetchMode and then iterating over the resulting PDOStatement object after an execute method has been used on it.
The basics of the problem:
<?php
// we have a PDOStatement object
$statement->setFetchMode(PDO::FETCH_CLASS | PDO::FETCH_PROPS_LATE, 'myClass');
$statement->execute(array($myargs));
foreach ($statement as $record) {
print_r($record);
}
?>
This won't respect the PDO::FETCH_PROPS_LATE flag and the $record may not display correctly.
If we want to use FETCH_PROPS_LATE we may need to do something like:
<?php$statement = db_query('SELECT * FROM {node}');
foreach ($statement->fetchALL(PDO::FETCH_CLASS | PDO::FETCH_PROPS_LATE, 'myClass') as $record) {
print_r($record);
}
?>
If we go this route PDO::FETCH_PROPS_LATE isn't needed in DatabaseStatement::execute. If we remove it from there I'd suggest documenting it's need and usage in detail or it will bite someone else the same way it bit me.
Thoughts?
#5
Ugh. The latter syntax is just way too ugly. This is definitely a bug in PHP from the sound of it. Rats.
I think the best way forward here is to document the hell out of it, and even link to the PHP bug in a docblock. I guess we'll just have to live with early fetch for Drupal 7, and if someone cares we need to make sure that they can explicitly ask for the late fetch.
Can we get a unit test to allow people to explicitly request late fetch and then setup the backend code and docs to ensure it works?
#6
Another option is to make db_query() return a subclass of PDOStatement that compensates for the PHP bug, so that the calling code will work as if the bug wasn't there. A workaround like this would take quite a bit of code, though, and it may also be slightly confusing to users that e.g. get_class($result) != "PDOStatement" (apart from being confusing when debugging this would only be a problem with poorly written code).
#7
@c96065 I have a feeling that using/needing PDO::FETCH_PROPS_LATE will be a fairly rare occurrence (please correct me if I'm wrong). I'm with @Crell on going without PDO::FETCH_PROPS_LATE by default and documenting how to use it if necessary.
#8
We're already using a subclass of PDOStatement, DatabaseStatement, for just such extensions. In fact we've implemented a bunch already. :-)
But yeah, we're not actually using the class-based fetching anywhere in core yet, so until this becomes a problem and we find we need it often I think it's better to just document the heck out of it. So we need a patch that 1) rolls back the late-fetch change and 2) Documents in docblocks this caveat. We can then add docs to the handbook, too.
#9
I'll write the patch and the handbook page unless someone else wants to own this.
#10
This patch reverts the change in #298309: DatabaseStatement::execute with PDO::FETCH_CLASS calls __construct() after setting object attributes.
#11
The patch does undo the change. Now we need a nice comment block explaining what's going on.
#12
Added documentation page at http://drupal.org/node/315092. I'll craft a patch up later this week with details.
#13
Here's a patch that removed FETCH_PROPS_LATE, points to the bug in a comment, and points to the handbook page on the comment. Please review the documentation and test the patch. It passed all the db tests for me.
#14
1. Comments need to wrap at 80 characters. I would just fix this myself, but...
2. Since this is primarily a documentation issue, can we add a sentence or two of "why"? All of the comments there are /factually/ correct, but don't tell me a) Why adding properties before a __construct() is a bad thing, b) why I might need to work around this problem.
#15
The attached patch wraps at 80 characters or less and adds a little more detail to the comment. I'm not sure how much detail to go into in the patch. If __construct() needs to do setup before properties are added to the object PDO::FETCH_PROPS_LATE needs to be used. Example cases include using the __set() and __get() magic methods to do something with the properties other than assign them to an object and what you are going to do with them needs to be setup. Should there be more detail? Should extra detail be added to the handbook page?
#16
This new patch has "will will" and "properties properties" in the code comments.
#17
oops oops. Good catch @keith.smith. Re-rolled
#18
The patch looks good.
A comment on the handbook page:
I think this behaviour is actually by design. The bug is that you cannot use PDO::FETCH_PROPS_LATE to switch to the reverse behaviour. I suggest dropping the last sentence.
AFAICT the PHP bug is only of concern to the user if he calls setFetchMode() himself - right? In that case it may not be worth mentioning (it may be confuse more than it helps), but if it is mentioned, I think that bug 41971 is also deserves mention, even though it has been fixed in the latest PHP 5.2.x release.
#19
@c960657 the handbook page definitely needs some work :)
#20
The code comment seems to presume that the user is asking why we're using early fetch. I suspect most users won't know the difference. I'd suggest instead something like "Default to an object. Note: db fields will be added to the object before the constructor is run. If you need to assign fields after the constructor is run, do X or see Y." (More formal than that, of course.) I'm flexible as to whether we mention the PHP bug here or on the handbook page or both.
#21
Better title.
#22
@Crell - I made the presumption. I updated to your comment in this patch. I'd suggest more detail as to why, how to set the fields after the constructor is run, and an example be in the handbook. This is a rare use case.
#23
I'm Larry Garfield, and I approve of this patch.
#24
Committed to CVS HEAD. Thanks.
--project followup subject--
Automatically closed -- issue fixed for two weeks with no activity.
#25
Automatically closed -- issue fixed for two weeks with no activity.