Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This is a followup for #1735118: Convert Field API to CMI and postponed on that issue.
The constructors for Field
and FieldInstance
provide default values for some $values
.
In Field
:
// Provide defaults.
$values += array(
'settings' => array(),
'cardinality' => 1,
'translatable' => FALSE,
'entity_types' => array(),
'locked' => FALSE,
'deleted' => 0,
'storage' => array(),
'indexes' => array(),
);
In FieldInstance
:
// Provide defaults.
$values += array(
'label' => $values['field_name'],
'description' => '',
'required' => FALSE,
'default_value' => array(),
'default_value_function' => '',
'settings' => array(),
'widget' => array(),
'deleted' => 0,
);
These defaults are buried inside the constructors, so it is not clear to calling code or developers what defaults are available.
Proposed resolution
Provide a static method on the class that returns these defaults.
Remaining tasks
- Create an initial patch that adds a static method to both classes.
- Update the constructors to use these static methods.
- The default for the field label in
FieldInstance
will need to be merged in afterward in the constructor since this is a dynamic value.
Comment | File | Size | Author |
---|---|---|---|
#17 | 1969032-17-field-defaults.patch | 6.08 KB | amateescu |
#13 | 1969032-13-field-defaults.patch | 5.66 KB | swentel |
#13 | interdiff.txt | 1.16 KB | swentel |
#8 | 1969032-field-defaults.patch | 5.69 KB | Cameron Tod |
Comments
Comment #1
abghosh82 CreditAttribution: abghosh82 commentedI would like to work on this one, however this is postponed and depends on another patch from issue #1735118. So can I go ahead and create the initial patch?
Comment #2
yched CreditAttribution: yched commentedActually, I've never been sure whether those defaults couldn't / shouldn't be set in the definition of each property (de-facto self-documenting them).
E.g :
Is there some specific reason why this would be discouraged in Entity classes ?
Comment #3
xjm@abghosh82, you'll want to wait until the other issue is committed and this one becomes active, because the code to patch isn't in the core codebase yet. :)
@yched, no reason I can think of. Sounds like a great idea to me. I think we could probably do both that and this. Only thing I can think of is I'm not sure member variables are set before
parent::__construct()
unless they are static. @abghosh82 can try a patch and find out. :)Comment #4
yched CreditAttribution: yched commentedMember variables are initialized before any method runs, including __construct(), I think. Any other behavior wouldn't sound realistic.
I'm more worried about a possible gotcha with entity api - but that might be totally ungrounded.
Not sure what you mean by "do both that and this". Default values on member declarations AND merge an array returned by a static method during __construct() ? Sounds like unneeded duplication to me ?
Comment #5
abghosh82 CreditAttribution: abghosh82 commentedThanks xjm, I'll wait for the issue to be active.
Sorry, I am not sure about the patch you want me to try, I'll patch Field and FieldInstance with some member variables and check if the value is set before parent::__construct().
However I think I agree with yched.
Comment #6
abghosh82 CreditAttribution: abghosh82 commentedI checked the member variable with Field class and it worked fine the values were set before parent::__construct().
Comment #7
yched CreditAttribution: yched commentedUn-postponing.
Comment #8
Cameron Tod CreditAttribution: Cameron Tod commentedHow's this? Too ugly on the label default maybe?
Comment #9
swentel CreditAttribution: swentel commentedThe label makes sense, don't have any big problems with it.
Comment #10
smiletrl CreditAttribution: smiletrl commentedThis looks good to me too, although I'm not sure is this the conventional way of doing this.
Only one thing:
Do we need
if
condition? Maybe we could do$this->label = $values['field_name'];
directly?Comment #11
swentel CreditAttribution: swentel commentedNo, because if there's a label supplied, then we'd overwrite this again
Comment #12
yched CreditAttribution: yched commentedif ($this->label == NULL)
should beif (!isset($values['label']))
But
would be better / leaner IMO :)
While we're in there, since this specific default for 'label' puzzled @xjm a bit in the Field / CMI patch, could we add a word of explanation in the phpdoc for the "public $label" . Something like :
Other than that, works for me !
Comment #13
swentel CreditAttribution: swentel commentedComment #14
yched CreditAttribution: yched commentedCool, thanks !
Comment #15
catch#13: 1969032-13-field-defaults.patch queued for re-testing.
Comment #17
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #18
webchickLooks like a nice clean-up.
Committed and pushed to 8.x. Thanks!