Problem

Two things:

A)
Field types need to be able to do some extra massaging on the "default values" when they get in and out of CMI storage.
- config for "default values" of entity_ref / taxo / image fields need to reference UUIDs instead of numeric ids - and this is critical for config deployment: see #1735118-217: Convert Field API to CMI (there is probably a more specific issue for this, but I couldn't find it)
- date module does strange tricks to handle its own default values - and this is currently broken: #1989468: Weird messing with 'default_value_function' in date widgets ?

B)
It's currently up to widgets to specify if they support "default field values".
When displaying the "field instance edit form", Field UI checks whether the widget used by the field supports default values, and if yes, displays the widget as an input for "default values".

This comes straight from CCK D6 and was introduced to let date fields provide their own handling of "default values" - because a default value for a date field is typicially "current date", "current date + 1 day"..., and those are not values that can be entered using any date widget, which only let you input a specific date ("12/25/2013").

This is conceptually broken:
- "the widget used by the field" makes no sense now that we have form modes
- the support from default values and how to enter them should be specific to the field type, not widget by widget

Proposed solution
Now that field types are finally classes, we can implement a sane flow around default values:
- add defaultValuesForm() / defaultValuesFormSubmit() methods in ConfigFieldInterface, just like there currently is settingsForm() and instanceSettingsForm() in ConfigFieldIItemnterface
The methods are added on the field classes rather than the field item classes, because this is where default value handling occurs, as per #2050801: Unify handling of default values between base and configurable fields and #1777956: Provide a way to define default values for entity fields.
- ConfigField::defaultValuesForm() reproduces the current behavior (display a widget to allow input of default values).
Field types than want to opt out or do things differently just override the method. Patch does that for file and image fields.
- defaultValuesFormSubmit() lets them process values before they get saved in CMI (e.g numeric id -> UUID)
- getDefaultValue(), added in #2050801: Unify handling of default values between base and configurable fields, lets them massage back default values before using them in actual "create new" entity forms (e.g UUID -> numeric id)

API changes
- this is mostly adding methods to ConfigFieldInterface, but base implementations in ConfigField reproduce the current behavior,
- 'default_value' property in widget plugin annotations is "officially" removed, will just have no effect if it's still there.

Files: 
CommentFileSizeAuthor
#50 default_value_UI-2056405-50.patch26.29 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,226 pass(es).
[ View ]
#50 interdiff.txt2.84 KByched
#48 default_value_UI-2056405-48.patch26.29 KByched
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#47 default_value_UI-2056405-47.patch26.31 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es).
[ View ]
#47 interdiff.txt24.14 KByched
#42 default_values-2056405-42.patch27.82 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,845 pass(es).
[ View ]
#42 interdiff.txt815 bytesplopesc
#40 default_values-2056405-40.patch28.58 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch default_values-2056405-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 interdiff.txt1 KByched
#35 default_values-2056405-33.patch27.78 KByched
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#33 default_values-2056405-33.patch27.78 KByched
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [default_values-2056405-33.patch] from [drupal.org].
[ View ]
#33 interdiff.txt5.54 KByched
#28 default_values-2056405-27.patch23.87 KByched
FAILED: [[SimpleTest]]: [MySQL] 57,592 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#28 interdiff.txt18.08 KByched
#23 default_values-2056405-23.patch24.23 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 57,960 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#23 interdiff.txt8.58 KBplopesc
#21 default_values-2056405-21.patch19.19 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,836 pass(es).
[ View ]
#21 interdiff.txt4.45 KBplopesc
#18 default_values-2056405-18.patch18.88 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,637 pass(es).
[ View ]
#18 interdiff.txt7 KBplopesc
#14 default_values-2056405-14.patch17.08 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 57,631 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt10.44 KBplopesc
#12 default_values-2056405-12.patch16.76 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,865 pass(es).
[ View ]
#12 interdiff.txt10.8 KBplopesc
#9 default_values-2056405-9.patch13.46 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,131 pass(es).
[ View ]
#9 interdiff.txt10.14 KBplopesc
#8 default_values-2056405-8.patch11.02 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,593 pass(es).
[ View ]
#4 default_values-2056405-4.patch31.09 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch default_values-2056405-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

yched’s picture

Issue tags:+API change, +Field API, +CMI

tagging

yched’s picture

Issue summary:View changes

Rephrase

plopesc’s picture

Assigned:Unassigned» plopesc

Working on this one...

Regards.

yched’s picture

Awesome - I was hoping someone would grab this one :-)

Be aware of #2050801: Unify handling of default values between base and configurable fields, it's going to be a part of the equation - the getDefaultValue() method added over there is how field types can override their own logic to generate actual default values from the stored configuration (like, UUID --> local numeric id, 'now' --> actual date...).

plopesc’s picture

Status:Active» Needs review
StatusFileSize
new31.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch default_values-2056405-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hello

First attemp. It just moves logic from FieldInstanceEditForm to ConfigFieldItemBase.

Some comments:

  • Added defaultValuesFormValidate method too.
  • It only works for field types that inherits from ConfigFieldItemBase, it should be implemented for ConfigEntityReferenceItemBase fields too.
  • I don't know if $this->instance property in FieldInstanceEditForm is now right or should be changed.

Moreover, I found a bug in the current default value behavior. Drupal is broken when you try to edit a field that is selected as 'hidden in the form modes tab. Is this a known bug?

Waiting for your feedback.
Regards

Status:Needs review» Needs work

The last submitted patch, default_values-2056405-4.patch, failed testing.

yched’s picture

re #5:
- $this->instance in FieldInstanceEditForm should be $this->getFieldDefinition() in FieldItem classes. Code should then only use FieldDefinitionInterface methods, no more direct access to $instance->someProperty (let me know if this causes trouble)
- "default value" broken when field is using the "hidden" widget: nope I don't think there's an issue for this. In this case, I think we should fall back to the "default_widget" for the field type.

yched’s picture

Also - patch #4 seems to contain a lot of unrelated changes from other patches in the queue :-)

plopesc’s picture

Status:Needs work» Needs review
StatusFileSize
new11.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,593 pass(es).
[ View ]

Oh, sorry
Lasts commits in HEAD broke my diff between branches.

Re-rolled

plopesc’s picture

StatusFileSize
new10.14 KB
new13.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,131 pass(es).
[ View ]

New round, using fieldDefinition in ConfigFieldItemBase class.

To avoid the hidden widget issue, I created a fallback that creates a new widget object that is passed to the form. Then , I needed to change the defaultValuesForm methods definition.

Removed fieldTypeManager and widgetManager properties in FieldInstanceEditForm because only were used in the getDefaultValueWidget method.

What do you think?

Regards

yched’s picture

Status:Needs review» Needs work

That looks like a very good start, let's see if we can push it a bit.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.phpundefined
@@ -148,6 +148,27 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+   * {@inheritdoc}
+   */
+  public function defaultValuesForm(array $element, array &$form, array &$form_state) {
+    return array();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function defaultValuesFormValidate(array &$form, array &$form_state) {
+    return array();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function defaultValuesFormSubmit(array &$form, array &$form_state) {
+    return array();

Those would need to duplicate the method bodies from ConfigFieldItemBase ? :-/
Although at this point, it might be easier to have ConfigEntityReferenceItemBase extend ConfigFielditemBase and duplicate the methods in EntityReferenceItem...
Let's leave it like that for now, I'll try to have @amateescu chime in.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,79 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+    $entity_form_display = $form['#entity_form_display'];

Then it seems $form['#entity_form_display'] is used only within the defaultValuesForm*() implementations here, and FieldInstanceEditForm has no business defining it and placing it in the form.
It's only an implementation choice of ConfigFieldItemBase : "use the widget configured for the entity in the 'default' form mode", for which it needs the $entity_form_display.

Suggestion:
Move the logic about "figure out the actual widget to use (grab the entity_form_display, grab the widget used for 'default mode', unless it's 'hidden' in which case fallback to the 'default_widget' of the field type)" to a protected defaultValueWidget() method, witch returns an instantiated widget plugin object.
Each step (form, formValidate, formSubmit) can call it without storing stuff in $form.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -274,34 +224,7 @@ protected function getDefaultValueWidget($field, array &$form, &$form_state) {

I tend to think we should get rid of FieldInstanceEditForm::getDefaultValueWidget(). Like, always call the FieldItem::defaultValueForm*() methods. It's up to the FieldItem classes to do something or not.

FieldInstanceEditForm::buildForm() calls defaultValueForm() and wraps it in the 'details' element ("The default value for this field, used when creating new content" help text...) only if the return value is not empty. This allows to get rid of the $element param in defaultValueForm()

At any rate, we need to get rid of the field_behaviors_widget() check in FieldInstanceEditForm::buildForm(), that logic is now moot.

yched’s picture

Side note: there was this issue about the bug with "hidden" widget: #2028759: Clean up instance['widget'] in Field UI
I linked to the current issue from over there.

plopesc’s picture

Status:Needs work» Needs review
StatusFileSize
new10.8 KB
new16.76 KB
PASSED: [[SimpleTest]]: [MySQL] 57,865 pass(es).
[ View ]

Hello

Attaching new patch including some suggestions, like defaultFormWidget method, get rid of $form['#entity_form_display'] and get rid of $element in FieldInstanceEditForm::getDefaultValueWidget() and FieldItem::defaultValueForm().

Sorry, but I can't continue today. This is a middle step patch you can check if you want. Tomorrow I will work further.

Regards

yched’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,100 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+    $default_widget = $this->defaultValueWidget();

Nitpick: just $widget would be more accurate / less confusing.

(same in the the other methods)

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,100 @@ public function defaultValuesFormValidate(array $form, array &$form_state) {
+    $element = $form['instance']['default_value_widget'];

OK, this is not ideal because it means the class has knowledge of the structure of the form generated by FieldInstanceEditForm - i.e "I know you called my defaultValuesForm() method and placed the result in $form['instance']['default_value_widget']". This kind of coupling is not too great.

Pondering this a bit...

plopesc’s picture

StatusFileSize
new10.44 KB
new17.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,631 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

New patch that includes the following changes:

  • Get rid of FieldInstanceEditForm::getDefaultValueWidget()
  • Get rid of field_behaviors_widget() check in FieldInstanceEditForm::buildForm()
  • defaultValuesForm*() are FieldInstanceEditForm agnostics given that the $element is passed as method parameter

We should address the ConfigEntityReferenceItemBase issue. I'll try to work later on it.

Regards.

Status:Needs review» Needs work

The last submitted patch, default_values-2056405-14.patch, failed testing.

yched’s picture

Great ! The test that fails precisely tests the behavior we're changing, so it can just be removed.
Instead, we need a test that "if widget is set to 'hidden', then the default widget is used instead"

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,96 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+    static $widget = NULL;

Let's not use static variables in object classes.
I propose the following:
Let defaultValueWidget() receive $form_state by ref, and do:

if (!isset($form_state['default_value_widget']) {
  $form_state['default_value_widget'] = ... $widget...;
}
return $form_state['default_value_widget']
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,96 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+      $entity_form_display = entity_get_form_display($this->getFieldDefinition()->entity_type, $this->getFieldDefinition()->bundle, 'default');

$this->getFieldDefinition() ->entity_type or ->bundle are not strictly kosher, since they are not part of FieldDefinitionInterface. They work here because those "defaultValueForm" methods will be called for configurable fields for now, so getFieldDefinition() returns a FieldInstance object that has the properties. But it's not great if we want to change that in the future.

Instead, you can use:
$entity = $this->getParent()->getParent();
$entity->entityType() / $entity->bundle().

Similarly, the other methods in ConfigFieldItemBase should do $entity = $this->getParent()->getParent(); rather read it from $form['#entity']. Let's avoid a tight couple on stuff the caller has placed stuff in $form.

(side note, I've long been willing to open an issue to add a getEntity() method as shorthand for getParent()->getParent()
on FieldItemInterafce but failed to do so so far - If you feel like it ;-)

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,96 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+      if (!$widget = $entity_form_display->getRenderer($field_name)) {

I don't think this will work, since the 'hidden' widget is an actual plugin.
Should be something like:
$widget = entity_get_form_display(...)->getRenderer($field_name);
if (empty($widget) || $widget->getPluginId() == 'hidden')) {
// fallback to the default widget
}

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,96 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+        $field_type = \Drupal::service('plugin.manager.entity.field.field_type')->getDefinition($this->getFieldDefinition()->getFieldType());
+        $widget = \Drupal::service('plugin.manager.field.widget')->getDefinition($field_type['default_widget']);
+        $configuration = array(
+          'field_definition' => $this->getFieldDefinition(),
+          'settings' => $widget['settings']
+        );
+        $widget = \Drupal::service('plugin.manager.field.widget')->createInstance($field_type['default_widget'], $configuration);
+      }

Could be reduced to just:
$widget = \Drupal::service('plugin.manager.field.widget')->getinstance(array('field_definition' => $this->getFieldDefinition());
getInstance() takes care of fetching the default widget and assigning its settings.

yched’s picture

Also:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -187,30 +160,8 @@ public function buildForm(array $form, array &$form_state, FieldInstanceInterfac
     if (isset($form['instance']['default_value_widget'])) {

This will always be set now, so the check can go away (same in submitForm())

plopesc’s picture

Status:Needs work» Needs review
StatusFileSize
new7 KB
new18.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,637 pass(es).
[ View ]

Hello

New step:

  • Removed static variable in defaultValueWidget()
  • Use of $entity = $this->getParent()->getParent() (Let's open a follow up issue after finish this oneto add the getEntity() method ;))
  • if (!$widget = $entity_form_display->getRenderer($field_name)) is right for now. When the widget is set to 'hidden', its reference is removed in form_display config entity and returns NULL. It returns the following error if you try to check getPluginID() in a hidden widget: Fatal error: Call to a member function getPluginId() on a non-object
  • Previously I tried to do it with no success. Now, I was debugging it deeper and I found that there is a problem if you don't set the optional "configuration" option. I added a check in WidgetPluginManager::getInstance()
  • Removed if (isset($form['instance']['default_value_widget'])) from code.

Remaining tasks:

  • Tests for the new hidden widget behavior.
  • Address the ConfigEntityReferenceItemBase behavior.

Regards.

yched’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -87,7 +87,7 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   public function getInstance(array $options) {
-    $configuration = $options['configuration'];
+    $configuration = isset($options['configuration']) ? $options['configuration'] : array();

Ah, indeed :-)
If we're doing this, then let's rather add this at the top of the function:

// Fill in defaults for missing properties.
$options += array(
  'configuration' => array(),
  'prepare' => TRUE,
);

Then the line after "// Fill in default configuration if needed." a couple lines below can be simplified to just if ($options['prepare']) {. Yay for cleaner code.

yched’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -117,25 +117,27 @@ public function defaultValuesFormSubmit(array $element, array &$form, array &$fo
+      if (!$form_state['default_value_widget'] = $entity_form_display->getRenderer($field_name)) {

Nitpick: keeping an intermediate $widget variable used to figure out the right $widget, then assigning it to $form_state[...] at the end would make clearer code.
Also, matter of taste, but we generally tend to favor readability over terseness, and unfold syntax like "test negation assignment" (if (!$var = ...)):

$widget = (plan a)
if (!$widget) {
  $widget = (plan b)
}
$form_state['...'] = $widget;
+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -160,9 +160,7 @@ public function buildForm(array $form, array &$form_state, FieldInstanceInterfac
-    if (isset($form['instance']['default_value_widget'])) {
       $this->getFieldItem($form['#entity'], $this->instance['field_name'])->defaultValuesFormValidate($form['instance']['default_value_widget'], $form, $form_state);

Indentation needs fixing then ;-)
(same in submit)

plopesc’s picture

StatusFileSize
new4.45 KB
new19.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,836 pass(es).
[ View ]

Cleaning code.

yched’s picture

Thanks for being so quick ! Stacking remarks till a next reroll is also fine if it's easier for you.
I add my remarks as I see stuff so that I don't forget them ;-)

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldItemBase.phpundefined
@@ -45,4 +46,101 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+      // When field is hidden, use the default widget and store it in the form.

Grammar is incorrect, and the comment is slightly misleading (we put the widget in the form state in all cases).
Maybe just a single comment on top of that code block : "Use the widget currently configured for the 'default' form mode, or fallback to the default widget for the field type" ?

plopesc’s picture

StatusFileSize
new8.58 KB
new24.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,960 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

Attaching patch that implements last suggested changes:

  • Add tests for the new hidden widget behavior.
  • Try to address the ConfigEntityReferenceItemBase behavior. As suggested in #10, now ConfigEntityReferenceItemBase extends from ConfigFieldItemBase and duplicate methods in EntityReferenceItem. I'm not sure if this is the best design or if it would be better duplicate the defaultValuesForm*() methods in the current ConfigEntityReferenceItem class, or maybe another different better approach.

Regards.

Status:Needs review» Needs work
Issue tags:-API change, -Field API, -CMI

The last submitted patch, default_values-2056405-23.patch, failed testing.

yched’s picture

Status:Needs work» Needs review

#23: default_values-2056405-23.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Field API, +CMI

The last submitted patch, default_values-2056405-23.patch, failed testing.

webchick’s picture

Issue tags:+Approved API change

Since this is a critical issue, if we need to re-jigger some APIs to fix this, we can mark this approved. Sounds like it should be pretty minimal, though, so great!

yched’s picture

Status:Needs work» Needs review
Issue tags:-Approved API change
StatusFileSize
new18.08 KB
new23.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,592 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Ok, sorry about that, we need to keep ConfigEntityReferenceItemBase extends EntityReferenceItem, because that's what triggers EntityReferenceItemNormalizer (runs on item classes that extend EntityReferenceItem).

So it means copying the defaultValuesForm*() methods. Sad panda...
There might be ways to reformulate that whole EntityReferenceItem inheritance class, but that's far outside the scope of this issue, so let's do this for now.

I refactored and shortened a couple things. Sorry @plopesc for "stealing" that last stretch, but some parts I needed to experiment myself, so it was easier that way.

yched’s picture

Issue tags:+Approved API change

don't steal our "approved" tag, d.o...

Side note: found #2060765: warning on "Field instance edit" form for a taxo field while testing this. Not related, though, happens in HEAD too.

plopesc’s picture

Hello
I'm glad to see it has been approved :)

Sorry @plopesc for "stealing" that last stretch, but some parts I needed to experiment myself, so it was easier that way.

No worries, now code has been polished and I can read it to see where I could have improved it :)

@yched Do you consider that we could open the follow-up issue you suggested in #16?

Regards

Status:Needs review» Needs work

The last submitted patch, default_values-2056405-27.patch, failed testing.

yched’s picture

Status:Needs work» Needs review

the follow-up about $entity = $this->getParent()->getParent() ?
Would be awesome :) A helper method for the language would be cool too.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new5.54 KB
new27.78 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [default_values-2056405-33.patch] from [drupal.org].
[ View ]

Field UI tests needed to be updated for the new structure of the form values.

In addition:
- put back "don't show anything if isset($instance->default_value_function)", as in HEAD
- reintroduced conditional execution of defaultValueFormValidate() / defaultValueFormSubmit(). Was needed by the previous point, and makes it easier for FieldItem classes to opt out (simply override defaultValueForm())
- make FileItem and ImageItem opt out, since those field types have their own handling of default values
- remove the "does not support default value" property on widgets; as per the OP, it's a behavior of the field type now

This should be ready now.
We need #2050801: Unify handling of default values between base and configurable fields before we can fix date.module

Status:Needs review» Needs work

The last submitted patch, default_values-2056405-33.patch, failed testing.

yched’s picture

Status:Needs review» Needs work
StatusFileSize
new27.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Hm, re-uploading

yched’s picture

Status:Needs work» Needs review

...

Status:Needs review» Needs work
Issue tags:-API change, -Field API, -CMI, -Approved API change

The last submitted patch, default_values-2056405-33.patch, failed testing.

yched’s picture

Status:Needs work» Needs review

#35: default_values-2056405-33.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Field API, +CMI, +Approved API change

The last submitted patch, default_values-2056405-33.patch, failed testing.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new1 KB
new28.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch default_values-2056405-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

damn annotations

Status:Needs review» Needs work

The last submitted patch, default_values-2056405-40.patch, failed testing.

plopesc’s picture

Status:Needs work» Needs review
StatusFileSize
new815 bytes
new27.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,845 pass(es).
[ View ]

Re-rolling patch.

Just removed some debugging code added to PathTaxonomyTermTest.

Regards.

plopesc’s picture

yched’s picture

Boy I was tired, that's debug code for another issue :-)

yched’s picture

Actually, while working on #2050801: Unify handling of default values between base and configurable fields again it occurred to me that the handling of "assign default values in newly created entities" happens in the Field classes, not the FieldItem classes.

So field types willing to modify how their default values are handled (eg. replace numeric IDs with UUIDs) will need to override methods in their 'list class' (extends Field) rather than in their 'item class' (extends FieldItem).

So it would make sense that the defaultValueForm*() methods live in ConfigFieldInterface / ConfigField rather than in ConfigFieldItemInterface / ConfigFieldItemBase.
- defaultValueFormSubmit() is the mirror method of the Field::getDefaultValue() method added in the other issue - massage before storing in config / massage after reading from config. If you implement one, you'll need to implement the other, so it's better DX if both live in the same interface & class.
- It would also make the code in the base implementations less weird: they currently live in the FieldItem object, but do most of their work on $this->getParent(), i.e the Field object. That just reflects the fact that "default values" are a concept that applies at the Field level, not FieldItem level.
- it would avoid duplicating the implementations in ConfigEntityReferenceItemBase, since they would live in ConfigField, which is the 'list' class entity_reference fields use.

Let's wait on #2050801: Unify handling of default values between base and configurable fields before refactoring this, it shouldn't be too far away.

mtift’s picture

Updating tags

yched’s picture

Issue summary:View changes

API changes

yched’s picture

StatusFileSize
new24.14 KB
new26.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es).
[ View ]

Now that #2050801: Unify handling of default values between base and configurable fields is in, and as per #45, moved the methods from the FieldItem classes to the Field classes.
I'll update the OP accordingly.

yched’s picture

StatusFileSize
new26.29 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Reroll

swentel’s picture

@@ -79,4 +79,88 @@ protected function getDefaultValue() {
+   *   The form_state array.

Should we do something like 'The form state of the (entire) configuration form.' like we do elsewhere ?

@@ -21,4 +21,53 @@
+   *   The default value form base element

Incredible nitpick: missing '.' at the end.

@@ -217,20 +174,15 @@ public function validateForm(array &$form, array &$form_state) {
+    if (isset($form['instance']['default_value'])) {
+      $items = $this->getFieldItems($form['#entity'], $this->instance['field_name']);
+      $default_value = $items->defaultValuesFormSubmit($form['instance']['default_value'], $form, $form_state);
+    }
+    else {
+      $default_value = array();
     }

Maybe put $default_value = array() before the if statement so we can drop the 'else'.

This is just matter of preference, I can live with it though.

yched’s picture

StatusFileSize
new2.84 KB
new26.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,226 pass(es).
[ View ]

Good call(s) :-)

swentel’s picture

Status:Needs review» Reviewed & tested by the community
webchick’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigFieldInterface.php
@@ -21,4 +21,53 @@
+  public function defaultValuesFormValidate(array $element, array &$form, array &$form_state);
...
+  public function defaultValuesFormSubmit(array $element, array &$form, array &$form_state);

This naming introduces an inconsistency with FormInterface, where we call these methods submitForm() and validateForm(). I don't really want to hold this patch up on this, since there are other places in core that are inconsistent as well like blockSubmit(), but I feel like we should open a follow-up to discuss unifying these names across all the various "here's how you define a form" interfaces. It might be that this gets kicked to D9, but we should see if the DX benefits are worth it.

+++ /dev/null
@@ -1,29 +0,0 @@
- * Definition of Drupal\field_test\Plugin\field\widget\TestFieldWidgetNoDefault.

I was wondering where this test code ran off to, but Alex pointed out that since this behaviour is now codified in an interface, the condition this test is checking for no longer happens. Yay for removing babysitting code! :)

Other than that, nothing really stood out to me as weird, so this looks good to go. Unfortunately I can't push anything right now because of https://drupal.org/node/2075775, but will try and get to this in my next commit spree if no one else beats me to it.

yched’s picture

re FormInterface():
I don't think FormInterface should be our concern here. A ConfigField class is not a form, it's a class that, as a *secondary* job, happens to provide a subform (currently one single subform, but ConfigFieldItem classes provide two different subforms: field settings form, instance settings form).
Those methods are about providing and processing a very specific subform which is "the default value form for this field type". Calling the methods buildForm(), validateForm(), submitForm() would be very misleading here.

webchick’s picture

Hm. Not sure. FormInterface is fairly common and so developers will have more experience with that than most other interfaces; seems like it makes sense to make "form-related thingies" consistent with that.

But in any case, hey, pushing works again! :D

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title:Let field types provide their support for "default values" and associated input UI» Change notice: Let field types provide their support for "default values" and associated input UI
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record
yched’s picture

Title:Let field types provide their support for "default values" and associated input UI» Change notice: Let field types provide their support for "default values" and associated input UI

Change notice added at https://drupal.org/node/2076489

I still disagree on #54.
- FormInterface is designed for forms, not subforms (we can't implement FormInterface::getFormID()
- seeing Field::buildForm(), Field::validateForm() + simply {@inheritdoc}... would be *very* misleading - "build a form for the field ?". We're only talking about "the UI for default values of the field", those are in now way inherent to "what is a Field, what would be its 'form'". Totally beats "developers will have more experience with FormInterface" IMO.
- what if we need to have those classes generate another, different subform ?

[edit: oh, signatures don't match anyway ;-) defaultValuesFormValidate() & defaultValuesFormSubmit() have an $element param - which is strongly tied to "being a subform"]

yched’s picture

Title:Change notice: Let field types provide their support for "default values" and associated input UI» Let field types provide their support for "default values" and associated input UI
Status:Active» Fixed
Issue tags:-Needs change record

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

Anonymous’s picture

Issue summary:View changes

methods live on Field, not FieldItem