-
Notifications
You must be signed in to change notification settings - Fork 829
PDO Fetch Modes #4936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
PDO Fetch Modes #4936
Conversation
…te to branch off the existing constants docs)
…basic modes complete)
…set changes for TSV alignment)
… combination list)
|
Wow, this is a lot of work. Thank you. I don't know when I will have the time to review this. I would say that you shouldn't be concerned too much with documenting broken or undefined behaviour. We can make issues for these and revisit them later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great. I think this is going to be a huge improvement. The only thing that feels weird is that the fetch modes are now on a separate page behind a link, but I have no idea how to present it better. Maybe "Predefined Constants" should be split into 3 pages with "cursors" and "other constants" getting their separate pages too?
| <?php | ||
| function valueCreator($col1, $col2, $col3) | ||
| { | ||
| $col2 = strtoupper($col2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why you have this twice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why I have what twice here? I'm not sure what you're pointing to and I can't see anything that's been duplicated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strtoupper($col2) twice. Here and two lines below
There are precedents for constants sections with separate pages - see openssl, posix and radius. (curl also has them split across multiple files in the docs repo but not in the published html docs). I considered having separate pages for "cursors" and "other" constants here, but I didn't think there is currently enough content to justify it (and was also consciously avoiding making "unrelated changes" as much as possible) |
| <para> | ||
| This fetch mode does not directly return a result, but binds values to | ||
| variables specified with <function>PDOStatement::bindColumn</function>. The | ||
| called fetch method returns <literal>TRUE</literal>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| called fetch method returns <literal>TRUE</literal>. | |
| called fetch method returns &true;. |
Wouldn't this be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine now. You can merge it.
Fixes #1425 (and numerous user comments on various pages)
The documentation for PDO fetch modes is currently distinctly lacking. There's some specific documentation and examples on various pages, but nothing decent "all in one place".
https://phpdelusions.net/pdo/fetch_modes has long been oft referenced by myself and by many others across chats and forums. If it ever goes missing for any reason we have nothing comparable in the official documentation.
This is my attempt to fix that. While my work is based on the above link at a high level, I have written this documentation "from scratch", performing my own tests to verify behavior, along with referencing php-src code and tests.
I have specifically written this documentation so that wherever the PDO::FETCH_* constants appear, you can click them and they take you right to the detailed docs.
There's a summary at the top of the page so people can still quickly see all the available fetch modes and jump to the detailed documentation.
Test scripts I wrote while writing this article: https://github.com/AllenJB/sandbox/tree/main/php-docs/pdo/fetch-modes
Undefined Behaviors
There are some behaviors I've currently documented as specifically undefined. These are:
I couldn't see any obvious tests that were explicitly testing these behaviors in php-src. If you think I should document that current behavior as the define behavior, please let me know.
FETCH_COLUMN | FETCH_GROUP
I've filed this as a bug: php/php-src#20215
I have specifically not documented the behavior of FETCH_COLUMN | FETCH_GROUP because I think it's non-sensical and probably isn't supposed to work the way it currently does.
To see what I'm talking about, please refer to https://github.com/AllenJB/sandbox/blob/main/php-docs/pdo/fetch-modes/column/group.phpt (3v4l version: https://3v4l.org/2f7aV )
When you use FETCH_COLUMN, you can specify the additional column parameter to fetchAll() to tell PDO which column to retrieve values from. However, when you combine it with FETCH_GROUP, the column number is used for the grouping key and the values returned are always the first column.
This does not match my personal expected behavior, especially given that the column parameter can only be used with the COLUMN fetch mode - no other fetch mode allows you to influence how GROUP works in this way.
It also does not match the current documentation for PDOStatement::fetchAll()
You can see from the 3v4l link above that COLUMN | GROUP works as I expected in PHP <5.2.6.
I could find no tests in php-src for the combination of COLUMN | GROUP and specifying the column parameter.
The following is a timeline I put together for this:
I have no idea what to do about the documentation here. The current documentation for PDOStatement::fetchAll is wrong, but the current code behavior is non-sensical in my opinion.