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.
Comment | File | Size | Author |
---|---|---|---|
#50 | default_value_UI-2056405-50.patch | 26.29 KB | yched |
#50 | interdiff.txt | 2.84 KB | yched |
#48 | default_value_UI-2056405-48.patch | 26.29 KB | yched |
#47 | default_value_UI-2056405-47.patch | 26.31 KB | yched |
#47 | interdiff.txt | 24.14 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedtagging
Comment #1.0
yched CreditAttribution: yched commentedRephrase
Comment #2
plopescWorking on this one...
Regards.
Comment #3
yched CreditAttribution: yched commentedAwesome - 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...).
Comment #4
plopescHello
First attemp. It just moves logic from
FieldInstanceEditForm
toConfigFieldItemBase
.Some comments:
defaultValuesFormValidate
method too.ConfigFieldItemBase
, it should be implemented forConfigEntityReferenceItemBase
fields too.$this->instance
property inFieldInstanceEditForm
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
Comment #6
yched CreditAttribution: yched commentedre #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.
Comment #7
yched CreditAttribution: yched commentedAlso - patch #4 seems to contain a lot of unrelated changes from other patches in the queue :-)
Comment #8
plopescOh, sorry
Lasts commits in HEAD broke my diff between branches.
Re-rolled
Comment #9
plopescNew 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 thegetDefaultValueWidget
method.What do you think?
Regards
Comment #10
yched CreditAttribution: yched commentedThat looks like a very good start, let's see if we can push it a bit.
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.
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.
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.
Comment #11
yched CreditAttribution: yched commentedSide 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.
Comment #12
plopescHello
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
Comment #13
yched CreditAttribution: yched commentedNitpick: just $widget would be more accurate / less confusing.
(same in the the other methods)
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...
Comment #14
plopescNew patch that includes the following changes:
FieldInstanceEditForm::getDefaultValueWidget()
field_behaviors_widget()
check inFieldInstanceEditForm::buildForm()
defaultValuesForm*()
are FieldInstanceEditForm agnostics given that the $element is passed as method parameterWe should address the ConfigEntityReferenceItemBase issue. I'll try to work later on it.
Regards.
Comment #16
yched CreditAttribution: yched commentedGreat ! 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"
Let's not use static variables in object classes.
I propose the following:
Let defaultValueWidget() receive $form_state by ref, and do:
$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 ;-)
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
}
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.
Comment #17
yched CreditAttribution: yched commentedAlso:
This will always be set now, so the check can go away (same in submitForm())
Comment #18
plopescHello
New step:
defaultValueWidget()
$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 checkgetPluginID()
in a hidden widget: Fatal error: Call to a member function getPluginId() on a non-objectWidgetPluginManager::getInstance()
if (isset($form['instance']['default_value_widget']))
from code.Remaining tasks:
ConfigEntityReferenceItemBase
behavior.Regards.
Comment #19
yched CreditAttribution: yched commentedAh, indeed :-)
If we're doing this, then let's rather add this at the top of the function:
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.Comment #20
yched CreditAttribution: yched commentedNitpick: 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 = ...)
):Indentation needs fixing then ;-)
(same in submit)
Comment #21
plopescCleaning code.
Comment #22
yched CreditAttribution: yched commentedThanks 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 ;-)
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" ?
Comment #23
plopescAttaching patch that implements last suggested changes:
ConfigEntityReferenceItemBase
extends fromConfigFieldItemBase
and duplicate methods inEntityReferenceItem
. 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.
Comment #25
yched CreditAttribution: yched commented#23: default_values-2056405-23.patch queued for re-testing.
Comment #27
webchickSince 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!
Comment #28
yched CreditAttribution: yched commentedOk, 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.
Comment #29
yched CreditAttribution: yched commenteddon'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.
Comment #30
plopescHello
I'm glad to see it has been approved :)
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
Comment #32
yched CreditAttribution: yched commentedthe follow-up about $entity = $this->getParent()->getParent() ?
Would be awesome :) A helper method for the language would be cool too.
Comment #33
yched CreditAttribution: yched commentedField 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
Comment #35
yched CreditAttribution: yched commentedHm, re-uploading
Comment #36
yched CreditAttribution: yched commented...
Comment #38
yched CreditAttribution: yched commented#35: default_values-2056405-33.patch queued for re-testing.
Comment #40
yched CreditAttribution: yched commenteddamn annotations
Comment #42
plopescRe-rolling patch.
Just removed some debugging code added to PathTaxonomyTermTest.
Regards.
Comment #43
plopescCreated follow-up issue suggested in #16 and #32:
#2061331: Add getEntity() helper in FieldInterface / FieldItemInterface
Comment #44
yched CreditAttribution: yched commentedBoy I was tired, that's debug code for another issue :-)
Comment #45
yched CreditAttribution: yched commentedActually, 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.
Comment #46
mtiftUpdating tags
Comment #46.0
yched CreditAttribution: yched commentedAPI changes
Comment #47
yched CreditAttribution: yched commentedNow 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.
Comment #48
yched CreditAttribution: yched commentedReroll
Comment #49
swentel CreditAttribution: swentel commentedShould we do something like 'The form state of the (entire) configuration form.' like we do elsewhere ?
Incredible nitpick: missing '.' at the end.
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.
Comment #50
yched CreditAttribution: yched commentedGood call(s) :-)
Comment #51
swentel CreditAttribution: swentel commentedComment #52
webchickThis 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.
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.
Comment #53
yched CreditAttribution: yched commentedre 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.
Comment #54
webchickHm. 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!
Comment #55
webchickComment #56
yched CreditAttribution: yched commentedChange 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"]
Comment #57
yched CreditAttribution: yched commentedComment #58.0
(not verified) CreditAttribution: commentedmethods live on Field, not FieldItem