Use early fetch and document why

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.

@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:

  // 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:

  $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?

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?

@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.

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.

I'll write the patch and the handbook page unless someone else wants to own this.

The patch does undo the change. Now we need a nice comment block explaining what's going on.

Added documentation page at http://drupal.org/node/315092. I'll craft a patch up later this week with details.

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.

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.

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?

This new patch has "will will" and "properties properties" in the code comments.

oops oops. Good catch @keith.smith. Re-rolled

@c960657 the handbook page definitely needs some work :)

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.

Better title.

@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.

I'm Larry Garfield, and I approve of this patch.

Committed to CVS HEAD. Thanks.