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:

<?php
  
// Provide defaults.
   
$values += array(
     
'settings' => array(),
     
'cardinality' => 1,
     
'translatable' => FALSE,
     
'entity_types' => array(),
     
'locked' => FALSE,
     
'deleted' => 0,
     
'storage' => array(),
     
'indexes' => array(),
    );
?>

In FieldInstance:

<?php
   
// 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.
Files: 
CommentFileSizeAuthor
#17 1969032-17-field-defaults.patch6.08 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 54,811 pass(es).
[ View ]
#13 1969032-13-field-defaults.patch5.66 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1969032-13-field-defaults.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 interdiff.txt1.16 KBswentel
#8 1969032-field-defaults.patch5.69 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es).
[ View ]

Comments

I 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?

Actually, 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 :

/**
  * The thingie for doing stuff.
  *
  * @var int
  */
public $foo = 0;

Is there some specific reason why this would be discouraged in Entity classes ?

@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. :)

Member 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 ?

Thanks 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.

I checked the member variable with Field class and it worked fine the values were set before parent::__construct().

Status:Postponed» Active

Un-postponing.

Status:Active» Needs review
StatusFileSize
new5.69 KB
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es).
[ View ]

How's this? Too ugly on the label default maybe?

The label makes sense, don't have any big problems with it.

This looks good to me too, although I'm not sure is this the conventional way of doing this.
Only one thing:

+    // Set field label to field name if no value has been set.
+    if ($this->label == NULL) {
+      $this->label = $values['field_name'];
+    }

Do we need if condition? Maybe we could do $this->label = $values['field_name']; directly?

Do we need if condition? Maybe we could do $this->label = $values['field_name']; directly?

No, because if there's a label supplied, then we'd overwrite this again

Status:Needs review» Needs work

if ($this->label == NULL) should be if (!isset($values['label']))

But

$values += array(
  'label' => $values['field_name'],
);

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 :

* If not specified, this defaults to the field_name (mostly useful for field instances created in tests).

Other than that, works for me !

Status:Needs work» Needs review
StatusFileSize
new1.16 KB
new5.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1969032-13-field-defaults.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Reviewed & tested by the community

Cool, thanks !

Status:Reviewed & tested by the community» Needs work
Issue tags:+DX (Developer Experience), +Novice, +Field API

The last submitted patch, 1969032-13-field-defaults.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 54,811 pass(es).
[ View ]

Rerolled.

Title:Add a static method defining the default values for fields and field instancesMore sensible way of defining field default values
Status:Reviewed & tested by the community» Fixed

Looks like a nice clean-up.

Committed and pushed to 8.x. Thanks!

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