It isn't necessary to insert default values in drupal_write_record() as the database already knows what they are.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, dont-insert-default-values.patch, failed testing.

jbrown’s picture

Ahh, empty fields in the object need to be populated with default values for the caller.

jbrown’s picture

Why is it ignored?

jbrown’s picture

Attempting with different filename.

Berdir’s picture

Status: Needs work » Needs review

Because the issue is set to needs work :)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think this makes, sense, if I understand it all correctly. :-)

jbrown’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.63 KB

Updated a comment.

Benefits of this patch:

  • Don't bloat queries unnecessarily - performance increase?
  • There is no longer an inconsistency between serialized defaults in database and those that drupal_write_record() writes - see http://drupal.org/node/585894#comment-2779846
jbrown’s picture

Updated patch.

Now $record will not be converted to an object if an exception occurs - #731554: drupal_write_record() silently converts an array to an object in case of an exception

jbrown’s picture

with new casting coding standards...

Crell’s picture

I'm OK with this assuming the bot is. If that fixes the other issue as well, score.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

OK, seems like Crell and the bot are good with it. Who could ask for more?

Looks OK to me. I tested and it resolves #731554: drupal_write_record() silently converts an array to an object in case of an exception and I do like this approach better.

aspilicious’s picture

#9: dont-insert-default-values.patch queued for re-testing.

aspilicious’s picture

*Retesting old patches*

jbrown’s picture

#9: dont-insert-default-values.patch queued for re-testing.

jbrown’s picture

Can this be committed?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a nice clean-up. I hope our test coverage here is good...

Committed to HEAD.

jbrown’s picture

Status: Fixed » Closed (fixed)

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