Problem/Motivation
In PostgreSQL there is a failure in testFieldMap() of the FieldInfoTestCase test caused by inconsistent order of the array elements returned by field_info_field_map() function. This function calls $cache->getFieldMap() which afterwards executes the SELECT query. This SELECT query does not use ORDER BY so that the order in the results array is not guaranteed.
Test failure looks like this:
testFieldMap
fail: [Other] Line 1553 of modules/field/tests/field.test:
Value array (
'field_2' =>
array (
'bundles' =>
array (
'test_cacheable_entity' =>
array (
0 => 'test_bundle',
),
'test_entity' =>
array (
0 => 'test_bundle',
),
),
'type' => 'hidden_test_field',
),
'field_1' =>
array (
'bundles' =>
array (
'test_entity' =>
array (
0 => 'test_bundle_2',
1 => 'test_bundle',
),
),
'type' => 'test_field',
),
) is equal to value array (
'field_1' =>
array (
'type' => 'test_field',
'bundles' =>
array (
'test_entity' =>
array (
0 => 'test_bundle',
1 => 'test_bundle_2',
),
),
),
'field_2' =>
array (
'type' => 'hidden_test_field',
'bundles' =>
array (
'test_entity' =>
array (
0 => 'test_bundle',
),
'test_cacheable_entity' =>
array (
0 => 'test_bundle',
),
),
),
).
Steps to reproduce
Run Field Info tests in PostgreSQL.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 3264826-5.patch | 973 bytes | mcdruid |
| #2 | 3264826-2.patch | 1.08 KB | poker10 |
Comments
Comment #2
poker10 commentedThis should be the easiest way to fix this without chanegs in the core. This patch fixes the test.
Comment #3
mcdruid commentedI think this looks pretty good.
I am wondering if we should add an ORDER BY to the query in
\FieldInfo::getFieldMapinstead of sorting the map in the test though.Comment #4
poker10 commentedYes it should be possible to solve it this way. I just wanted to touch the core code only if really needed, so that's the reason why I changed the test only.
Comment #5
mcdruid commentedYup, fair enough.
If this doesn't break tests for the other dbs though, any reason not to fix it this way?
I suppose it could cause (minor?) regressions on some PostgreSQL sites, but I'm thinking that greater consistency between the dbs is generally better for everyone.
Comment #6
poker10 commentedPatch #5 looks good, no new failures and the targeting fail is gone.
However I am not sure if this cannot cause some regressions also in MySQL. In case that there are no explicit ORDER BY clauses then PostgreSQL does sorting according to its optimizer. I do not know exactly how MySQL sorts rows in such case. So maybe the resulting array order can change also in some MySQL/SQLite databases. But the real impact should depend on how this function is really used - if it is only helper function to get field definition, or something really depends on the order of returned bundles/entities by this query.
There are only two usages of
getFieldMap():field_info_field_map()is used only in testsFieldInfo->prepareField()seems to be used in coreComment #7
mcdruid commentedGood digging, thanks.
IIUC the sort order in MySQL is essentially "undefined" when there's no ORDER BY clause, and the behaviour could easily be different between storage engines (e.g. MyISAM / InnoDB) or MySQL versions. Example stackexchange reference (not necessarily definitive).
So I'd argue it's better to introduce an ORDER BY clause and ensure greater consistency between db backends (even within the different dbs that use the mysql driver).
The tests continue to pass with this change for MySQL and SQLite, which is good. However the test doesn't exercise the functionality in great depth.
I think we should stick to changing just the tests when that makes sense as it's only really the test that behaves differently for PostgreSQL (e.g. the database.schema.table prefixing issue we looked at recently), but in cases like this I'd prefer to make Drupal behave consistently than massage the test to make it pass.
One for Framework Manager review.
Comment #9
mcdruid commentedDiscussed with Fabianx in chat before commit.