Replace direct access to FieldConfigBase::default_value with methods

Rolling the ball. FieldConfigBase::getDefaultValue($entity) already exists, so it's hard to name properly how to get the default value for the field config. Suggestions welcome.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -284,11 +284,12 @@ public function hasNewEntity() {
   public static function calculateDependencies(FieldDefinitionInterface $field_definition) {
...
-    if (is_array($field_definition->default_value) && count($field_definition->default_value)) {
+    $default_value = $field_definition->getTheDefaultValue();

getTheDefaultValue() has been added to FieldConfigInterface, but maybe should it be on FieldDefinitionInterface?

Changing a missed reference to the property.

+++ b/core/modules/comment/comment.module
@@ -143,11 +143,12 @@ function comment_theme() {
 function comment_field_config_create(FieldConfigInterface $field) {
...
+    $default_value = $field->getTheDefaultValue();
+    if (!isset($default_value)) {
...
+    $default_value += array(array());
+    $default_value[0] += array(
       'status' => CommentItemInterface::OPEN,

Maybe now it's possible to make comment field default value with this method... follow-up

Fix failures, missed some accesses to the variable.

+++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
@@ -94,6 +94,19 @@ public function setSetting($setting_name, $value);
   /**
+   * Gets the set default value.
+   *
+   * Note that if a default value callback is set, it will take precedence over
+   * any value here.
+   *
+   * @return mixed $value
+   *   The default value in the format as returned by
+   *   \Drupal\Core\Field\FieldDefinitionInterface::getDefaultValue().
+   *
+   */
+  public function getTheDefaultValue();

So FieldDefinitionInterface already has a getDefaultValue() (no 'the'). Then FieldConfigInterface has setDefaultValue() which defines its parameter as the output of FieldDefinitionInterface::getDefaultValue().

FieldConfigBase then implements FieldDefinitionInterface, which as you can see accesses default_value according to some logic.

This means that FieldConfigBase::getDefaultValue() and FieldConfigBase::setDefaultValue() already implements the encapsulation we desire, or at least should.

This patch should not define getTheDefaultValue() and should instead focus on having calling code use the interface rather than the public method.

FieldDefinitionInterface::getDefaultValue receives an FieldableEntityInterface, which sometimes we don't have if working directly with field definitions. That's why I defined a new getter. That entity is needed because we call later to ::processDefaultValue, which needs the entity.

Should we ignore that if the entity is null?

I guess I really don't know. :-)

Having a setDefaultValue() and also a setTheDefaultValue() seems rather confusing in terms of DX.

Let's try allowing NULLs then.

Oops, rebased with 8.0.x

Yay

So, summarizing:

The runtime default value of a field can be specified in two different ways, each with its associated setter :
- default_value / setDefaultValue() : a litteral value,
- default_value_callback / setDefaultValueCallback() : a callback, possibly accepting the host $entity as a param.

Those are currently combined in a single "getter", getDefaultValue(), whose intent is to compute the runtime default value for a given field in a given entity, based on one of the two underlying properties (callback if present, else litteral). It is thus not really a getter for the two basic properties above, instead it smushes/resolves them to a runtime litteral.
And there are no real getters for the underlying properties (litteral / callback)

--> I'd suggest we keep getDefaultValue() for what it currently is, and add separate, dedicated getters for those two params :
FieldDefinitionInterface::getDefaultValueLitteral() / getDefaultValueCallback(),
with proper docs stating that getDefaultValue($entity) should be used instead for determining the actual runtime default value for a given field in a given entity.

Then we'd have :
setDefaultValue($value) : set as litteral value
setDefaultValueCallback($callback) : set as a callback
getDefaultValueLitteral() : get the litteral value
getDefaultValueCallback() : get the callback
getDefaultValue($entity) : get/compute the runtime default value for an entity

It's not entirely symmetric (setDefaultValue() is the setter for which getDefaultValueLitteral() is the getter), but IMO that's still the most intuitive DX solution. I'm not sure we want to rename setDefaultValue() to setDefaultValueLitteral() at this point.

Also, I notice that FieldConfigBase currently misses the setDefaultValueCallback() setter for default_value_callback (BaseFieldDefinition has it). That was probably an overlook in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods, we could add it here as well.

Makes sense to me, wondered if we wanted to really expose those methods.
This patch covers what yched asked for if I understood correctly.

Thanks @penyaskito ! A couple remarks :

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
   @@ -94,6 +94,19 @@ public function setSetting($setting_name, $value);
   +   * Gets the set default value.
   +   *
   +   * Note that if a default value callback is set, it will take precedence over
   +   * any value here.
   +   *
   +   * @return mixed $value
   +   *   The default value in the format as returned by
   +   *   \Drupal\Core\Field\FieldDefinitionInterface::getDefaultValue().
   +   *
   +   */
   +  public function getDefaultValueLiteral();
   

   Why do we redefine this here ? It's already defined in FDI, which FCI extends.

   What we need to add in FCI is setDefaultValueCallback(), see next item below :-)

3. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -520,6 +534,14 @@ public function setDefaultValue($value) {
      /**
       * {@inheritdoc}
       */
   +  public function setDefaultValueCallback($value) {
   

   Great, but the @inheritdoc is a lie atm :-), the method is not defined/documented in any interface currently implemented by FieldConfigBase

   We need to add it to FieldConfigInterface, that's where the various setters live atm.

+++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
@@ -230,6 +230,20 @@ public function getTargetBundle() {
+  public function getDefaultValueLiteral() {
+    return $this->default_value;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getDefaultValueCallback() {
+    return $this->default_value_callback;
+  }

Minor, code organisation : the rest of the class tries to group getters and setters together (getters first), let's do the same here.

4. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -179,6 +179,30 @@ public function getDisplayOptions($display_context);
   +   * If you want the runtime default value, you should call ::getDefaultValue
   

   Not sure of our standards here, but shouldn't we add a trailing () after the method name ?

   For completion and clarity, I'd suggest that crossreference getDefaultValueLiteral(), getDefaultValueCallback(), getDefaultValue() all cross-reference each other. Something like :

   - getDefaultValueLiteral() / getDefaultValueCallback()
   "This method retrives the raw property assigned to the field definition. When computing the runtime default value for a field in a given entity, ::getDefaultValue() should be used instead."

   - getDefaultValue() :
   "This method computes the runtime default value for a field in a given entity. To access the raw properties assigned to the field definition, ::getDefaultValueLiteral() / ::getDefaultValueCallback() should be used instead.

5. +++ b/core/modules/comment/comment.module
   @@ -143,11 +143,12 @@ function comment_theme() {
   +    $default_value = $field->getDefaultValueLiteral();
   +    if (!isset($default_value)) {
   

   The isset() looks weird now that we extract a variable. An is_null() would be more explicit ?

   In fact, now that we have getters / setters, we could ensure that we always get and set array() instead of NULL, and remove that check here.

   The phpdoc in HEAD for FieldDefinitionInterface::getDefaultValue() already states that it always returns an array, empty for "no default".
   The phpdoc for the newly added FieldDefinitionInterface::getDefaultValueLiteral() says the same, so it should already be the case ?

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -284,11 +284,12 @@ public function hasNewEntity() {
   +    if (is_array($default_value) && count($default_value)) {
   

   If we enforce the getter always returns an array, the is_array() check shouldn't be needed, and we could just do
   if ($default_value = $this->getDefaultValueLitteral())

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -305,19 +306,23 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +    $default_value = $field_definition->getDefaultValueLiteral();
   +    if (!empty($default_value)) {
   

   Same here, could be if ($default_value = ...). And the "if ($changed)" block at the end should move inside that first if(), after the foreach().

8. +++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
   @@ -61,7 +61,8 @@ function testCommentDefaultFields() {
   -    $this->assertEqual($field->default_value[0]['status'], CommentItemInterface::CLOSED);
   +    $default_value = $field->getDefaultValueLiteral();
   +    $this->assertEqual($default_value[0]['status'], CommentItemInterface::CLOSED);
   

   Minor, but we don't really *need* to extract a var, this could stay inlined. Feel free to ignore :-)

This one should make @yched happier :-)

Thanks @penyaskito. A couple more details :-)

1. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -430,6 +430,20 @@ public function isDisplayConfigurable($display_context) {
   +  public function getDefaultValueLiteral() {
   +    return isset($this->definition['default_value']) ? $this->definition['default_value'] : NULL;
   

   Actually, according to the convention and PHPdoc, the "not set" case should return [] rather than NULL ?

   + just like you did in FieldConfigBase, the getDefaultValue() method in BaseFieldDefinition could call getDefaultValueLiteral() rather than duplicating that line ?

   (actually, if getDefaultValue() only relied on getDefaultValueLiteral() / getDefaultValueCallback(), it could be moved to a trait and shared between FieldConfigBase & BaseFieldDefinition. Let's call that a followup though, I think there are more common runtime logic that could be shared in a trait between the two)

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -403,19 +403,61 @@ public function getDefaultValue(FieldableEntityInterface $entity) {
   +  public function setDefaultValue($value) {
   +    if (!is_array($value)) {
   +      $key = $this->getFieldStorageDefinition()->getPropertyNames()[0];
   +      // Convert to the multi value format to support fields with a cardinality
   +      // greater than 1.
          $value = array(
   -        array($property => $value),
   +        array($key => $value),
          );
   

   That's incorrect for an incoming $value === NULL, this will end up assigning a default value with NULL in the first property.

   We should be converting NULL to [] here, as getDefaultValueLitteral() does - in fact, no need to do it in both places, it should be enough to do it here in the setter.

   Likewise, no need to do the "normalize scalar to array using the first property" processing in both the getter and setter, doing it in the setter should be enough (we probablky need to keep it in getDefaultValue() that does "compute callback or litteral", since it currently allows the callback to return NULL or a scalar, we don't want a BC break here)

   Similar "non-duplicate processing / single-point of entry" logic should probably be checked in the equivalent methods in BaseFieldDefinition too ?

Oh, good catch. I double-checked that the default value is an empty array and it cannot be set to a NULL value.
Removed the logic in getters and reviewed it on setters on FileConfigBase and BaseFieldDefinition.

@yched So this is an API change, or should we allow NULLs then? I'm not sure I got it right.

@penyaskito: we should keep allowing NULL, but have the setter internally translate it to [] (empty array) before assigning it to a definition property. That preserves BC.

@yched: The test that fails is because after that, getting the value will return array() instead of NULL, is that right and should we change the test, or if the property is an empty array we should be returning NULL?

Yes, the getter returns [] for "no default value", that is what its phpdoc says.
"empty array" is the official value for "no default value". The setter accepts NULL instead for BC, but internally we always store [].

Changed the test expected output then.

Thanks @penyaskito ! Looks good.

This will likely need a change record though.

1. Yep we need a CR. It's good to see @yched's involvement in this issue. I like the current solution and I think it makes default value handling of fields more robust.

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -146,7 +146,7 @@
   -  public $default_value = array();
   +  protected $default_value = array();
   

   Is there no $default_value_callback property to protect? Hmmm... yes and it is already protected. We still have public access of it...

   if (empty($this->getFieldDefinition()->default_value_callback)) {
   

   in Drupal\Core\Field\FieldItemList::defaultValuesForm() and Drupal\datetime\Plugin\Field\FieldType::defaultValuesForm(). Is this broken atm?

3. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -419,6 +411,47 @@ public function getDefaultValue(FieldableEntityInterface $entity) {
   +  public function setDefaultValueCallback($value) {
   +    $this->default_value_callback = $value;
   

   $value should be $callback like the interface.

#29.2 - indeed. That doesn't fatal out because it's inside an empty() check, which eats the incorrect property access and always returns TRUE (see http://3v4l.org/9A8aB), but the underlying functionnality (don't show the UI to set a litteral default value when there is a default value callback) is indeed broken.

Fixed both access to default_value_callback. Should we have tests covering that?
Fixed also the argument name.

Drafted a change record.

Yeah, I guess we miss a test that checks that if a FieldConfig has a default_value_callback set, then the "Edit Field" UI doesn't show the "default value" input widget :-/

WIP of the test for that. There are some errors related to providing config schema, but aside of that it should work.

few minor points:

1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -418,6 +410,47 @@ public function getDefaultValue(FieldableEntityInterface $entity) {
   +  public function getDefaultValueLiteral() {
   ...
   +  public function setDefaultValue($value) {
   

   Minor: Would be nice to have both setter/getter same naming. Do you see anything wrong with getDefaultValue()?

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -418,6 +410,47 @@ public function getDefaultValue(FieldableEntityInterface $entity) {
   +      if ($value === NULL) {
   +        $value = [];
   +      }
   

   Do we really need this check? If the value is empty, don't we get warning already?

   <?php
   
   class Foo {
       protected $bar;
       public function setBar($value) {
           $this->bar = $value;
       }
   }
   
   $object = new Foo();
   $object->setBar();
   
   // Result:
   Warning: Missing argument 1 for Foo::setBar(), called in /in/kG7ps on line 11 and defined in /in/kG7ps on line 5
   
   Notice: Undefined variable: value in /in/kG7ps on line 6
   

   May be we can just stick with old isset()

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -306,18 +306,21 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +      if ($changed) {
   +        $field_definition->setDefaultValue($default_value);
   +      }
   

   Doesn't look like this is scope of this issue.

4. +++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
   @@ -60,8 +60,8 @@ function testCommentDefaultFields() {
   +    $this->assertEqual($field->getDefaultValueLiteral()[0]['status'], CommentItemInterface::CLOSED);
   

   is it safe to assume that 'status' going to be there?. it's not new change, just wondering...

5. +++ b/core/modules/field/src/Tests/FieldDefaultValueCallbackTest.php
   @@ -0,0 +1,96 @@
   +    /** @var FieldStorageConfig $field_storage */
   ...
   +    /** @var FieldConfig $field_config */
   

   Minor: thought we follow full class name?

6. +++ b/core/modules/field/src/Tests/FieldDefaultValueCallbackTest.php
   @@ -0,0 +1,96 @@
   +    $this->assertNoFieldByName('default_value_input[field_test][0][value]', 'Calculated default value', 'The default field form is visible.');
   

   visible? for 'No field'

7. +++ b/core/modules/field/src/Tests/FormTest.php
   @@ -613,7 +613,7 @@ function testHiddenField() {
   +    $this->field->setDefaultValue(array());
   

   May be we can add resetDefaultValue() ?
   Note: passing just NULL as argument set it as array.

Thanks for reviewing, Vijay!

1. getDefaultValue already exists and have logic, so we need something for accessing the raw value. See #16.
2. We allow NULL values as a default value, so API consumers could pass that. Internally that must be translated to an empty array.
3. Probably, but makes no harm as this line was already modified. Do you want to change it back?
4. Well, does are tests and we now comment value columns, so I would say it is safe.
5. Good find, changed it.
6. Oh, thanks. Changed the assert text.
7. I don't think it will be a common use case aside of using it on tests.

Vijay, maybe you could take a look at the test error? It is related to config schema and I'm not really sure how we should define the field config schema.

@penyaskito, I guess, the save() just returns status (ref: \Drupal/Core/Config/Entity/ConfigEntityStorage.php:299).

@vijaycs85: I didn't expect that inconsistency between content entities and config entities.
However, I changed that while doing some cleanup, I expect #39 to fail for the said reason: config schema of the field.

I didn't expect that inconsistency between content entities and config entities.

It doesn't look like content entity returns the object either:

Drupal/Core/Entity/ContentEntityStorageBase.php:211

 protected function doSave($id, EntityInterface $entity) {
   ...
    if ($entity->isNew()) {
   ...
      $return = SAVED_NEW;
    }
    else {
      $return = $entity->isDefaultRevision() ? SAVED_UPDATED : FALSE;
    }
   ...
    return $return;
  }

Anyways, let's see what testbot says.

@vijaycs85 Sorry for my confusion and the misunderstanding. What I would love you to look is at the error we have in the latest one:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for field.field.node.article.field_test with the following errors: field.field.node.article.field_test:default_value.value variable type is string but applied schema class is Drupal\Core\Config\Schema\Mapping in Drupal\Core\Config\Testing\ConfigSchemaChecker->onConfigSave() (line 98 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php). 

Thanks!

sorry for the delay @penyaskito. Looks like we got two problems. one with default_value and another one with default_value_callback.

1. default_value is an easy one. It just need another array before the ['value' => 'value']. you can see this at Drupal/Core/Field/FieldConfigBase::setDefaultValue()

public function setDefaultValue($value){
....
      // Convert to the multi value format to support fields with a cardinality
      // greater than 1.
      $value = array(
        array($key => $value),
      );


}

2. There is an assumption that the callback should be a string:

core/config/schema/core.data_types.schema.yml
field_config_base:
  type: config_entity
  mapping:
    id:
...
    default_value_callback:
      type: string
      label: 'Default value callback'
...

I can see this has been implied at Drupal/Core/Field/BaseFieldDefinition.php

  public function setDefaultValueCallback($callback) {
    if (isset($callback) && !is_string($callback)) {
      throw new \InvalidArgumentException('Default value callback must be a string, like "function_name" or "ClassName::methodName"');
    }

I tired to make the callback as static and represent the method as string 'ClassName::method', but it's throwing error:

Class 'Drupal\simpletest\WebTestBase' not found	PHP Fatal error	FieldDefaultValueCallbackTest.php	20	Unknown	

Anyway, I am attaching 2 patches, one without callback test and all green and with callback test which is failing.

getDefaultValue() uses call_user_func() for the default_value_callback, and yes, the setting has to be a string, so either a regular function name or 'namespacedClass::staticMethod' should work.

So I'm not sure why $field_config->setDefaultValueCallback('Drupal\field\Tests\FieldDefaultValueCallbackTest::calculateDefaultValue')->save();in @vijaycs85's 2529034-44-with-callback-fail.patch doesn't pass. I'd suggest trying with an absolute namespace (adding a leading '\' before 'Drupal\field\...') ?

Note that this will slightly conflict with #2552799: FieldType with no available widget causes Fatal error. Depending on which gets in first, the other one will need a reroll

Tried my suggestion in #46 :

Drupal\field\Tests\FieldDefaultValueCallbackTest :
-    $field_config->setDefaultValueCallback('Drupal\field\Tests\FieldDefaultValueCallbackTest::calculateDefaultValue')->save();
+    $field_config->setDefaultValueCallback('\Drupal\field\Tests\FieldDefaultValueCallbackTest::calculateDefaultValue')->save();

but that still causes the same fatal as @vijaycs85 describes in #44 : "Class 'Drupal\simpletest\WebTestBase' not found in FieldDefaultValueCallbackTest.php line 20"

If put the static callback in *another* class than the test class itself (say, FieldConfigBase, random pick) things work fine :
- if I manually edit field.field.node.article.body to put :
default_value_callback: 'Drupal\Core\Field\FieldConfigBase::calculateDefaultValue'
then the default value works as expected (node/add/article shows "Calculated default value" prefilled in the Body widget)
- If I modify the test with :
$field_config->setDefaultValueCallback('Drupal\field\Tests\FieldDefaultValueCallbackTest::calculateDefaultValue')->save()
then the test is green.

I tried using a static method in a different class in the same FieldDefaultValueCallbackTest.php file, that fails as well.
So it looks like this is only when referring to a class in the test file itself... Weird.

Fixed that by moving the method to a different class in a separate file.

Green, I'll allow myself to set back to RTBC since I only contributed a small test fix in the last patch.

I'm happy with it too. RTBC+1

I think this is the final public property on a config entity in core and as such it is a complete outlier and making it protected before we get to RC seems worth the probably limited disruption it will cause.

Committed dd8f566 and pushed to 8.0.x. Thanks!

Yay, that's one nice WTF going away. Thanks @penyaskito !

:/ https://www.drupal.org/pift-ci-job/31301

@jibran Sorry for that. https://www.drupal.org/node/2546598 should be helpful.

Adjusted the wording of the CR a bit :-)