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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

abghosh82’s picture

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?

yched’s picture

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 ?

xjm’s picture

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

yched’s picture

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 ?

abghosh82’s picture

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.

abghosh82’s picture

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

yched’s picture

Status: Postponed » Active

Un-postponing.

Cameron Tod’s picture

Status: Active » Needs review
FileSize
5.69 KB

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

swentel’s picture

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

smiletrl’s picture

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?

swentel’s picture

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

yched’s picture

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 !

swentel’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
5.66 KB
yched’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thanks !

catch’s picture

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.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.08 KB

Rerolled.

webchick’s picture

Title: Add a static method defining the default values for fields and field instances » More 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.