A small bug exists in drupal_write_record() failing to convert an array passed in as $object back to array format should drupal_get_schema() fail for any reason. Although drupal_get_schema() is not meant to return FALSE for any reason drupal_write_record() is still meant to live up to expectation and leave $object in it's original state. Patch attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

triclops’s picture

+1

sammys’s picture

Status: Active » Needs review

Bumping status...

Anonymous’s picture

FileSize
1.3 KB

updated patch to account for code committed here.

Pancho’s picture

Title: drupal_write_record() will not keep $object's original form (array/object) if drupal_get_schema() returns FALSE » Failing drupal_write_record() doesn't keep $object's original array form
Version: 6.0-beta4 » 6.x-dev

+1. This looks very good.

Anonymous’s picture

Bump.

Patch still applies cleanly to head. Is there a reason this is not going in other than time?

Anonymous’s picture

FileSize
1.29 KB

Updated patch to keep up with HEAD.

chx’s picture

Reason: we need people to test this.

cburschka’s picture

Patch broken by a trivial conflict. Here's a re-roll.

As an aside, does this concept code actually work?


function convert_object(&$array) {
  $array = (object) $array;
}

$test = array('a' => 'b');

convert_object($test);

if (is_object($test)) print "PHP clearly handles references in a different way than Java.";

Because that's what this "avoid confusing the caller" code seems to do. Does (object) and (array) casting work in-place in spite of the assignment?

sammys’s picture

Thanks for the reroll. :) PHP does work that way. Here is what happens when I execute your code above with PHP on the command line.

sammys@pie:~$ php -r 'function convert_object(&$array) {
  $array = (object) $array;
}

$test = array("a" => "b");

convert_object($test);

if (is_object($test)) print "PHP clearly handles references in a different way than Java.\n";'
PHP clearly handles references in a different way than Java.
sammys@pie:~$
sammys’s picture

Version: 6.x-dev » 6.1

bumping to 6.1

yched’s picture

Version: 6.1 » 7.x-dev

This should be bumped to D7, actually.
Should be fixed there first, then backported

vladimir.dolgopolov’s picture

Here is the test for Simpletest module.
I cannot reproduce the situation here: http://drupal.org/node/207170 since drupal_write_record() generate a valid query for core shemas.


class TestCase200824 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[200824] Failing drupal_write_record() doesn\'t keep $object\'s original array form'),
      'desc' => t('A small bug exists in drupal_write_record() failing to convert an array passed in as $object back to array format should drupal_get_schema() fail for any reason. Although drupal_get_schema() is not meant to return FALSE for any reason drupal_write_record() is still meant to live up to expectation and leave $object in it\'s original state.'),
      'group' => t('Drupal 7 Tests'),
    );
  }
  
  function testIssue() {
    $array = array();
    $object = new stdClass();

    // try to write record into not exsisted table
    drupal_write_record($this->randomName(), $array);
    drupal_write_record($this->randomName(), $object);

    $this->assertTrue(is_array($array), 'Test original array form');
    $this->assertTrue(is_object($object), 'Test original object form');
  }

}
Gábor Hojtsy’s picture

@Vladimir, how is a random name results in valid queries? Or do I misunderstand something here?

vladimir.dolgopolov’s picture

The point is:

"Although drupal_get_schema() is not meant to return FALSE for any reason drupal_write_record() is still meant to live up to expectation and leave $object in it's original state."

So if drupal_get_schema() fails (no schema found because of a random name) then drupal_write_record() won't convert "array" to "object" in any case.

bjaspan’s picture

Status: Needs review » Reviewed & tested by the community

I re-discovered this bug, applied the patch from this issue to D6, reviewed it, and it looks fine to me.

Vladimir's test case is currently sitting in this issue, not in a file anywhere. We do not yet have a plan for where to put test cases for individual bug fixes/issues. Once we figure that out, we can put his test case there.

Gábor Hojtsy’s picture

Committed to Drupal 6, still to be committed to 7!

Dries’s picture

Component: database system » tests
Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD. Thanks.

I'm update the component to 'tests' so we can still write a test for this (if necessary).

bjaspan’s picture

I discussed this with Gabor yesterday. Vladimir already wrote a test case, posted in this issue. I wasn't sure where to put it in the filesystem. It's a test for a particular bug fix, in this case in the includes/ directory. Can we have includes/includes.test? Or should we create a directory of per-issue test cases like ./tests/200824.test? Or something else? we;re going to end up having a lot of "here's the test for the bug reported in issue #xyz" and we need a consistent strategy for handling them.

sammys’s picture

Thanks for getting this committed everyone!

dropcube’s picture

Component: tests » base system
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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