As yched wrote in http://drupal.org/node/366377#comment-1229895 (#366377: Default values):
Since we now have a clean definition of default value as an instance / widget-agnostic property, then I think we're good to go with saving default values on insert. That automatically includes programmatic entity_save(), and form submission where the current user is not allowed to edit the field.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | field_save_default_value-392696-17.patch | 796 bytes | yched |
| #14 | field_save_default_value-392696-15.patch | 13.48 KB | yched |
| #12 | field_save_default_value-392696-11.patch | 13.35 KB | yched |
| #8 | field_save_default_value-392696-8.patch | 13.37 KB | yched |
| #3 | field_save_default_value-392696-4.patch | 9.27 KB | yched |
Comments
Comment #1
yched commentedAttached patch implements this. Comes with a test.
Comment #2
bjaspan commentedNice! Comments:
* Typo: "ampty array"
* Your tests assume the field is stored by field_sql_module. Other field attach tests do this, and there is an issue to fix them not to. I'd prefer you do a field_attach_load and look at the object you get back instead of querying the db, unless there is some reason that won't be a sufficient test.
* The PHPdoc for field_attach_insert() should note the new behavior of inserting defaults AND that the passed $object will be modified to contain them; or we should clone the object so as not to modify the passed object.
Comment #3
yched commented- fixed typo
- test separation : right. I was unsure how to handle this right now, but agreed, we're better off doing this cleanly for new tests. New patch also updates the existing testFieldAttachSaveMissingData(), from which the tests for default value were heavily derived.
- Good point about the modified $object. I'd say we're ok adding the default values to the $object. Not sure what other drupal API func we should look for consistency. drupal_write_record() adds serial keys, dunno if that's a good example. I don't see serious consequences right now, so I think since we have the information, we can as well make it available.
I'm not sure of the preferred way to convey that information in the PHPdoc. Suggestions / improvements welcome.
- patch also includes a needed addition in field_test.module that I forgot to include in the previous patch.
Comment #4
yched commentedComment #5
karens commentedThis looks good to me, but I haven't tested it yet. It is important to have a way to get default values working correctly, so this is important. And it is actually a tiny patch, only 9 lines of real code, everything else is documentation or tests.
Bookmarking this so I can get back to this today to test it, but the code looks good to me.
Comment #6
karens commentedOK, I have now tested it in a couple of ways, including using the CCK UI to create a default value function and saving it with node_save() and the default value seems to work correctly.
Comment #8
yched commentedPatch rerolled after #368674: Implement hybrid storage for Fields in Core, and accounts for a few remarks webchick made on IRC while reviewing :
- made the tests a bit cleaner by starting with a fresh $entity object on each step.
- added a PHPdoc for field_default_insert()
- cleans up a gotcha in the way _field_invoke() reassigns $items to $object->$field_name, avoiding clumsy workarounds in the newly added field_default_insert() and in the existing field_default_extract_form_values().
Comment #9
dries commentedI spent 20 minutes looking at this patch and it looks good. I'm still learning the Fields API so it would be good if someone could look into this too.
(I found it slightly confusing that field_attach_load() does not actually return data -- it returns data by reference but I'm sure that is actually better.)
Comment #10
yched commentedDries : field_attach_load() doesn't return results because it is a 'multiple load' function, just like node_load_multiple(), so it acts on the $objects.
This specific patch does nothing around load, though :-)
Comment #12
yched commentedRerolled. Back to CNR to let the bot run the tests.
Comment #14
yched commentedFix test exceptions introduced by the reroll. Thus, back to RTBC.
Comment #15
dries commentedOK, reviewed it again and it looks good. Committed to CVS HEAD. Thanks.
Comment #16
yched commentedThanks Dries.
The chunk in field_test.module was left out, though - breaks test bot. Here's a separate patch.
Comment #17
dries commentedCommitted. Use the wrong command line -- sorry.