Problem/Motivation

The Simpletest-specific table prefix handling code in _drupal_bootstrap_database() includes the following test:

elseif (is_array($value['prefix'])) {
  $current_prefix = $value['prefix']['default'];
}

Obviously, there's an invalid assumption there--that just because $value is an array it therefore has a value at index "default". If it's wrong, it causes test exceptions.

Note: The problem no longer exists in Drupal 8.

Proposed resolution

Make the logic smarter.

Remaining tasks

Review.

User interface changes

n/a

API changes

n/a

CommentFileSizeAuthor
#8 drupal.simpletest-table-prefixing-fix.2205817-8.patch840 bytesTravisCarden
#6 drupal.simpletest-table-prefixing-fix.2205817-6.patch885 bytesTravisCarden
#3 added_condition_to_check_default_key_in_prefix_array-2205817-3.patch729 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Title: Naive test in table prefixing code can lead to exceptions » Naive test in table prefixing code causes test exceptions
Assigned: TravisCarden » Unassigned
Issue summary: View changes
Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented

Yup, the DB layer seems to have the following code:


    if (is_array($prefix)) {
      $this->prefixes = $prefix + array('default' => '');
    }
    else {
      $this->prefixes = array('default' => $prefix);
    }

Which means that a non-set 'default' key in the array will just be silently ignored, apart from in _drupal_bootstrap_database() as you mention.

I'm not sure if Simpletest is capable of testing that this issue is there/has been fixed etc. I guess if we test DB prefixing somewhere, then we should be able to test this too.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
added_condition_to_check_default_key_in_prefix_array-2205817-3.patch729 bytes

I added condition to check the existence of 'default' key in $value['prefix'] array. If it does not exists fall back to Empty String

JayeshSolanki’s picture

JayeshSolanki CreditAttribution: JayeshSolanki commented
Status: Active » Needs review
JayeshSolanki’s picture

JayeshSolanki CreditAttribution: JayeshSolanki commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Reviewed & tested by the community » Needs review
FileSize
drupal.simpletest-table-prefixing-fix.2205817-6.patch885 bytes

That probably works, but I think it compensates for a defect in the code rather than fixing it. Attached is a solution that I think fixes the root problem and results in simpler, more readable code.

Status: Needs review » Needs work

The last submitted patch, 6: drupal.simpletest-table-prefixing-fix.2205817-6.patch, failed testing.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs work » Needs review
FileSize
drupal.simpletest-table-prefixing-fix.2205817-8.patch840 bytes

Oops; backwards patch. Take two.

davic’s picture

davic CreditAttribution: davic at CI&T commented
Status: Needs review » Reviewed & tested by the community

#8 Tested and is working fine, and does it smoothly. Seems to be a good approach.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Do we have an example where this snippet should apply, or even better, a test?

elseif (isset($value['prefix'])) {
  $current_prefix = $value['prefix'];
}
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Reviewed & tested by the community » Needs review