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

CommentFileSizeAuthor
#5 3264826-5.patch973 bytesmcdruid
#2 3264826-2.patch1.08 KBpoker10

Comments

poker10 created an issue. See original summary.

poker10’s picture

Status: Active » Needs review
StatusFileSize
new1.08 KB

This should be the easiest way to fix this without chanegs in the core. This patch fixes the test.

mcdruid’s picture

I think this looks pretty good.

I am wondering if we should add an ORDER BY to the query in \FieldInfo::getFieldMap instead of sorting the map in the test though.

poker10’s picture

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

mcdruid’s picture

StatusFileSize
new973 bytes

Yup, 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.

poker10’s picture

Patch #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 tests

FieldInfo->prepareField() seems to be used in core

mcdruid’s picture

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

  • mcdruid committed 2f66013 on 7.x
    Issue #3264826 by poker10, mcdruid: FieldInfoTestCase::testFieldMap...
mcdruid’s picture

Status: Needs review » Fixed

Discussed with Fabianx in chat before commit.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.