Other users are experiencing this same bug: drupal.org/node/1565384
When using ->fetchAll() method like this:
$query->fetchAll(PDO::FETCH_CLASS, 'my_class_name');
the second parameter ($fetch_column) is the class name of objects returned into result array.
Actual DatabaseStatementPrefetch.fetchAll() implementation does not work like this, and second parameter is simply ignored (it works only for PDO::FETCH_COLUMN mode).
File /includes/database/prefetch.inc, line 425:
public function fetchAll($fetch_style = NULL, $fetch_column = NULL, $constructor_args = NULL) {
$this->fetchStyle = isset($fetch_style) ? $fetch_style : $this->defaultFetchStyle;
$this->fetchOptions = $this->defaultFetchOptions;
if (isset($fetch_column)) {
$this->fetchOptions['column'] = $fetch_column;
}
//...
should read:
public function fetchAll($fetch_style = NULL, $fetch_column = NULL, $constructor_args = NULL) {
$this->fetchStyle = isset($fetch_style) ? $fetch_style : $this->defaultFetchStyle;
$this->fetchOptions = $this->defaultFetchOptions;
if (isset($fetch_column)) {
if ($fetch_style & PDO::FETCH_COLUMN) {
$this->fetchOptions['column'] = $fetch_column;
} else if ($fetch_style & PDO::FETCH_CLASS){
$this->fetchOptions['class'] = $fetch_column;
}
}
//...
This bug appeared on my side after installing the i18n_strings module, and it prevents nodes deletion.
Function i18n_string_load_multiple in i18n_string.inc(456) calls fetchAll with PDO::FETCH_CLASS parameter, and should return an array of 'i18n_string_object' classes, but returns stdClass instead.
I think this bug is related to Drupal core and not i18n project, because the latter calls ->fetchAll() correctly (but it doesn't work as expected):
// file /sites/all/modules/i18n/i18n_string/i18n_string.inc
return $query->execute()->fetchAll(PDO::FETCH_CLASS, 'i18n_string_object');
The attached patch fixes the bug.
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | drupal_fetchAll_classname_8.x_007.patch | 3.09 KB | nicorac |
| #20 | drupal_fetchAll_classname_8.x_006.patch | 2.46 KB | nicorac |
| #18 | drupal_fetchAll_classname_8.x_005.patch | 2.14 KB | nicorac |
| #13 | drupal_fetchAll_classname_8.x_004.patch | 1.01 KB | nicorac |
| #8 | drupal_fetchAll_classname_003.patch | 872 bytes | nicorac |
Comments
Comment #2
nicorac commentedPatch format error, resubmitting...
Comment #3
nicorac commentedOther user issue link: here
Comment #5
nicorac commentedPatch format error, resubmitting...
Comment #7
damien tournoud commentedApparently this is actually documented now... I'm pretty sure it wasn't when we first wrote this.
So we need:
DatabaseStatement::fetchAll()Comment #8
nicorac commentedPatch format error, resubmitting...
Comment #10
nicorac commentedWell, I'm full of it!
Can't get where the patch format error is:
--> Output: [fatal: patch fragment without header at line 3: @@ -423,13 +423,17 @@
Maybe someone can get it to work
Comment #11
aspilicious commented1) Failing because: "Ensure the patch applies to the tip of the chosen code-base"
2) And this needs to be fixed in drupal 8 first...
3) Don't bump issues to ciritcal without any proper reason
Comment #12
aspilicious commentedAnd we didn't do what Dalien told us to do in #7
Comment #13
nicorac commentedFine, will send a patch against 8.x-dev.
But is this a suggestion or a must?
Since I'm required to select a version when sending the patch, I supposed that the testbot will test my patch against the version I choose. And that's confirmed by testbot failure log: "[06:43:04] Main branch [7.14] checkout [complete].".
Again, who's in charge for backporting?
Actually v.7.14 and also 8.x-dev have no docblock at all for this function, so I don't think I'm the right person to start it from scratch (a maintainer is better).
I have no knowledge about Drupal test suite so I'm not able to create tests at all, sorry.
Comment #14
aspilicious commentedWell, yes we need (it's a must) to fix this first in drupal 8. We are al responsible for the backport, you me and the rest of the community. If you can't write the tests you can learn it or wait untill someone is going to write one.
If there are no tests this wont get committed soon.
If you want to push this you should try to look for existing tests
and try to add the failing cases.
Comment #15
damien tournoud commentedWe actually do, but it's in
DatabaseStatementInterface, notDatabaseStatement.Comment #16
nicorac commentedCan't find
DatabaseStatementInterfacedefinition, do you meanStatementInterface(which is actually the interface implemented by DatabaseStatement)?.It seems that
DatabaseStatementInterfacewas renamed and a lot of references left unchanged.Anyway
StatementInterfacedoesn't have FetchAll() defined.Comment #17
damien tournoud commentedSee core/lib/Drupal/Core/Database/StatementInterface.php, the definition of fetchAll() is there (but commented out for technical reasons).
Comment #18
nicorac commentedAdded docbook documentation to StatementInterface.
Comment #20
nicorac commentedChanged parameter names of StatementInterface.fetchAll() method.
Comment #22
nicorac commentedFixed
fetchAllparameters to comply with PHPfetchAll()definition.Changed both StatementInterface and its implementations.
Comment #23
catchTagging.
Comment #24
catchMarked #1854752: Re-Add PDO::FETCH_PROPS_LATE to PDO::FETCH_CLASS as duplicate.
Comment #25
bomoko commented#22: drupal_fetchAll_classname_8.x_007.patch queued for re-testing.
Comment #26
catchThat was re-opened because it covers more cases, marking this as duplicate then since we don't need two issues for this #1854752: Re-Add PDO::FETCH_PROPS_LATE to PDO::FETCH_CLASS.
Comment #27
chx commented#26 suggests this (and I agree, I guess catch just forgot to actually update the status).
Comment #27.0
chx commentedfixed link url