Make sure all settings are present at run time
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | normal |
| Assigned: | yched |
| Status: | closed |
| Issue tags: | Fields in Core |
For DX sake, the Field API tries to make sure that $field and $instances always contain all the expected (field|instance|widget|formatter) settings, avoiding the need for $foo = isset($field['settings']['foo']) ? $field['settings']['some_setting'] : 'default_for_foo'; code all over the place.
Currently, field_create_field() and field_create_instance() add missing settings on field / instance creation.
However, by the time we read the definition back, the execution context might have changed:
- the module implementing the field type, widget or formatter has been updated and has a new setting
- a 3rd party module, using the new *_info_alter hooks (#502522: Allow drupal_alter() on the various Field API declarative hooks) to add new settings, has been enabled.
The attached patch :
- makes sure missing settings and their default values are filled in field_read_fields() / field_read_instances() (was a TODO so far)
- adds some tests for this : testReadField(), testReadFieldInstance()
- reworks the existing testCreateField(), testCreateFieldInstance() so that they only test field_create_*(), not field_read_*()
- burns a kitten's whiskers by unifying minor things between field and instances CRUD test classes, and adding missing PHPdocs.test functions.
| Attachment | Size |
|---|---|
| field_settings_on_read.patch | 13.53 KB |
| Testbed results | ||
|---|---|---|
| field_settings_on_read.patch | failed | Failed: 11577 passes, 1 fail, 1 exception Detailed results |
#1
Er, typo in the sample ugly-code-we-want-to-avoid :
$foo = isset($field['settings']['foo']) ? $field['settings']['foo'] : 'default_for_foo';, of course.#2
The last submitted patch failed testing.
#3
Fix test error.
#4
The last submitted patch failed testing.
#5
Rerolled, moved a few kittens over to #518412: Minor cleanup in field.test
Would be cool to get in, there are other cleanups pending in this area ;-)
#6
Note to reviewers / core committers: the actual patch is like 6 lines, the rest is test adjustments :-)
#7
This patch increases the consistency of Field API by making field and instance objects more complete in more places.
#8
Committed to CVS HEAD. Thanks.
#9
This fails on the PIFR #2 clients :(
The change:
-class FieldInstanceTestCase extends DrupalWebTestCase {+class FieldInstanceCrudTestCase extends DrupalWebTestCase {
Because of the name change, when the tests are run this causes a PHP exception which is not capture by the testbot.
Running the tests via the CLI/PHP script, reports:
#10
@philipnet: thanks for the report, but I'm a little confused.
This patch is now committed. So do you mean that current HEAD fails on PIFR #2 because a test class name is different from what it was 3 days ago ?
If so, I'd be tempted to call this a bug in PIFR ?
#11
Sorry, my bad :(
It looks like the registry in D7 cached the old class name and the PHP script tried to call that. After a fresh install of HEAD I can't replicate the problem.
#12
Automatically closed -- issue fixed for 2 weeks with no activity.