Problem/Motivation
With #2226267: Improve default value handling of fields to be consistent the default value handling got improved, but the patch did not cover converting the default value implementations of uuid, created and changed fields.
We should convert those field types to use the new API as well, such that you can ask their definitions about the default values also.
uuid, language, created and changed field types currently use the Typed data way of setting default values, which basically is deprecated by the field definition way. Thus I think we should
- make DataDefinitionInterface follow the default value approach of FieldDefinitions + make applyDefaultValue() just use that (separate issue: #2318607: Improve default value handling for data definitions)
- We could support specifying a base default-value per field type, such that Created+Changed could just it to NOW + add that in FieldDefinition::getDefaultValue(). TimestampItem should support NOW as default value, as DateTime does. See DateTimeFieldItemList.
So this is definitely not a final version : I left these questions in the patch for reviewers.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#51 | interdiff.txt | 1.33 KB | yched |
#51 | 2318605-field_type_default_values-51.patch | 11.18 KB | yched |
Comments
Comment #1
fagoComment #2
fagoThis will have to cover "language" as well once #1966436: Default *content* entity languages are not set for entities created with the API goes in. Also tagging "beta deadline" as we have to give proper default value examples in core, such that modules can apply the pattern.
Comment #3
fagoGiven there is no API break or so, target probably fits better. Still, it should really happen before beta.
Comment #4
yched CreditAttribution: yched commentedLeft a note in #2150511-74: [meta] Deduplicate the set of available field types - in short, I don't get why we have the 'created' and 'changed' field types.
More specifically, the 'created' field type only exists to be able to specify REQUEST_TIME as a default value for what is otherwise a regular 'timestamp' field.
If we rework default values, should we make it possible to assign "NOW" as a default value for a 'timestamp' FieldDefinition ?
Comment #5
fagoyep, that what I've been thinking. It's a good thing to have generally and fits perfectly into processDefaultValue() of its field item list.
Comment #6
andypostComment #7
andypostIs there any examples? There weekend sprint coming so would be nice to attack
Comment #8
andypostrelated issues uses
FieldItemList::processDefaultValue()
so it looks all items will require to implement corresponding classesComment #9
fagoYep, except that for timestamp's I think it's enough to do it once for TimeStampItem (see patch).
There is no example yet, but here is first start of the implementation I've discussed with yched.
The idea is to add a "default_value" annotation to field type's which then is picked up if no default value is provided. For dynamic value the field type has to implement processDefaultValue() on the field item class.
Attached patch does that for TimestampItem + Create/Changed, but I did not test it at all. It misses completion for Langauge and UUID fields, i.e. those miss the ItemList class + processDefaultValue() implementation.
If you like, it would be great if you could pick it up from here. I'm at the sprint or IRC if there are questions.
Comment #11
fgmRerolled, fixed and completed. Some tests which would not pass with the previous version are now passing. Not sure everything's good, though:
\Drupal::languageManager()->getDefaultLanguage()->getId();
but maybe$entity->language()->getId();
would make more senseComment #12
fgmRerolled according to fago's insights : removed the comments, and the applyDefaultValue() methods on item classes.
Comment #15
fgmThis version should have /less/ fails, but probably still some.
Comment #17
yched CreditAttribution: yched commented"The default value assigned to the field" is not too clear in that context.
Maybe change to "The default value for fields of this type" ?
Should we also explain something like :
Individual field definitions can specify their own "default value", which will then take precedence on the value specified here. This thus can be seen as a default "default value" for fields of this type.
@var mixed means we need to explain what is accepted here - an example could also help for how to write that in annotations.
Those two pieces of code should be exactly the same, right ?
One does if (isset($definition['default_value']), the other does not - looks like this check is needed, since the 'default_value' entry is optional in the annotation ?
OK, so this adds support for 'NOW' as a "default value" for timestamp field types - Great :-)
But BaseFieldDefinitions can specify "allowed values" in several shapes :
The phpdoc for BaseFieldDefinitions::setDefaultValue($value) says: "$value can be specified in the same formats returned by FDI::getDefaultValue()"
FDI::getDefaultValue() says :
"This can be either:
- a literal, in which case it will be assigned to the first property of the first item.
- a numerically indexed array of items, each item being a property/value array.- NULL or array() for no default value"
Looks like this code here will only catch the 1st shape, which works for our tested case here because it's what CreatedItem's @FieldType annotation does.
But looks like it would fail to catch
Same remark for the other cases added here (CURRENT_LANGUAGE / LanguageItem, RANDOM / UuidItem).
In short : that polymorphism in the way you can specify allowed values is great for UX in BaseFieldDefinition::setDefaultValue(), and was kind of OK since it matches what $item_list->setValue() accepts at the end of the chain, but it is problematic the moment you need to do some masaging on that mixed input in between. We should add a step that always expands the 1st shape into the 2nd shape, so that processDefaultValues() implementations that need to do this kind of massaging always work with a predictable shape.
This could be done :
- in the base implementation of FieldItemList::processDefaultValues(), but that only works if child implementations think of calling the parent first.
- by the caller of ::processDefaultValues(), before the call. That would allow better documenting the @param and @return of FieldItemListInterface::processDefaultValues(), but has the drawback that the code that calls ::processDefaultValues() is duplicated in BaseFieldDefinition::getDefaultValue() and FieldConfigBase::getDefaultValue() :-(
Comment #18
fgmRerolled to take into account
- part of 1) (don't know how to document the mixed return, since it's basically anything)
- 2)
and hopefully fix one test, which tested for an undefaulted timestamp which is now defaulted.
Comment #20
fgmMore test fixes : because created now defaults to NOW, the install user gets included in the online users.
Comment #21
yched CreditAttribution: yched commented@fgm : interdiffs ? :-)
Regarding #17.3 :
Actually, I think we can consider this ("implementations of processDefaultValue() are currently supposed to do their job on data whose shape can vary") as an existing issue in the current API. This patch just adds implementations of processDefaultValue() that do not take that polymorphism into account, but the existing implementations that exist in HEAD (e.g in DateTimeFieldItemList or EntityReferenceFieldItemList) are currently equally broken, in that they assume they receive $default_value in a specific shape, which might not be the case.
So I think it's fine to proceed here, and fix processDefaultValue() in a separate issue. I'll try to open one soon.
Comment #23
yched CreditAttribution: yched commentedOpened #2347711: FieldItemlListInterface::processDefaultValue($default_value) is expected to massage polymorphic data
Comment #24
fgmMore fails removed. Don't forget (like I did) that in PHP, 0 == 'NOW' .
Comment #26
fgmOne down ?
Comment #28
fgmRerolled to match HEAD class renaming.
Comment #30
fgmAs discussed with yched and gabor, removing the language support from this patch, as this is another can of worms which warrants its own followup issue.
Interdiff is with the #24 patch, not the latest one.
Comment #31
fagoLooks like some unrelated changes sneaked in. Else the patch looks good to me.
Comment #32
fgm@fago : as I discuessed with @yched, this is actually related : prior to this patch, the timestamp information on the install user were not initialized, and now they are, like for any other user. Better consistency, IMO.
In turn, this causes the install user to be listed in the Who's online block, so the test needs to account for it.
Comment #33
yched CreditAttribution: yched commentedLike @fago said - the changes in testUserLoginBlock() just seem to be testing more things (adding checks about the $install_user). Not sure why this patch means we need to test more things there ?
Other than that, RTBC.
We should open an issue for the "default language" part @fago described in #3.
Comment #34
fgm@yched : as I wrote above, the patch causes the install user to be part of the "who's online" block (not the testUserLoginBlock as git would have you believe), because its timestamps are now correctly initialized, which they never were previously, so I think the tests needs to be modified to reflect this.
Comment #35
fgmAdded comment per our offline discussion.
Comment #36
yched CreditAttribution: yched commentedThanks!
Comment #37
fagoI see - thanks! Agreed this is RTBC, but re-uploading to make sure that exactly the committed patch has been tested. No changes!
Comment #39
fgm@fago that looks like an entirely different patch ?
Comment #40
fagoops, sry. Re-uploading the same file again seems to be a too hard task for me ;-) Next try. (No changes!)
Comment #41
yched CreditAttribution: yched commentedBot seems a bit distracted, but RTBC when green
Comment #43
BerdirTestbot ate the previous patch, uploading again, wasn't able to trigger a restart on testbot side. No commit credit :)
Comment #44
BerdirWas RTBC, testbot can set it back if he disagrees.
Comment #45
alexpott#2347711: FieldItemlListInterface::processDefaultValue($default_value) is expected to massage polymorphic data landed - allegedly this needs work.
Comment #48
yched CreditAttribution: yched commentedOops, I expected it would be the other way around :-)
Basically, the changes needed here is that processDefaultValue() now receives $default_value as an array keyed by delta, e.g :
The implementations added in this patch need to be adjusted to do their massaging in that format - basically, doing a foreach ($default_value as $delta => &$item_value) { process $item_value }
Comment #49
yched CreditAttribution: yched commentedHere's the updated patch.
The interdiff hunks in getDefaultValue() implementations are about doing things in the right order :
- get the default value as specified in the field definition
- if not present, get it from the field type (added by this patch here)
- normalize it to an array (added by #2347711: FieldItemlListInterface::processDefaultValue($default_value) is expected to massage polymorphic data)
- pass it to processDefaultValue()
Comment #51
yched CreditAttribution: yched commentedNULL needs to be normalized to array() if we want processDefaultValue() implementations to just foreach (also, the phpdoc for processDefaultValue() says it receives an array). That's more a bug from #2347711: FieldItemlListInterface::processDefaultValue($default_value) is expected to massage polymorphic data :-/
Comment #52
yched CreditAttribution: yched commentedThinking a bit more about uuids:
UuidItem fields defaulting to a newly generated value duplicates the code that currently assigns UUIDs on entity_create() :
That one runs for both config and content entities, while the feature added here only scopes content entities - and is actually never leveraged, since the Storage handler makes sure a value is specified for the uuid field before it reaches the place where ItemList::assigndefaultValue() is called.
Seems confusing to have two places that try to create UUIDs on new entities, one of which never actually runs ?
Moreover - what if I want a field that stores a UUID as a reference / foreign key ? I wouldn't want a "newly generated UUID" to be applied as a default value there. Then I need to specify ->setDefaultValue(NULL) or something ?
In the end, I'm not fully convinced that adding that as "the default value for all fields of this type" really helps. *If* we really want the generation of entity primary UUIDs to go through the "default values" flow (and, as stated above, that is currently not the case even with this patch), then why don't we leave it to each field definition to specify $field['uuid'] = new BFD('uuid')->setDefaultValue('RANDOM') ?
Comment #53
yched CreditAttribution: yched commentedOh gee. That code in EntityStorageBase::create() actually only runs for config entities, because ConfigEntityStorage defines $this->uuidService and ContentEntityStorageBase doesn't. Not exactly intuitive :-/
Why don't we rather unify this for config and content entities, then ?
Comment #54
fagoBumped into this while working on #2405943: User entity validation misses form validation logic in conjunction with languages. Added missing language field to the issue summary and updating title.
Also I think this is major because it requires us to do this:
This effectively makes the *valid* default value of NULL for language fields being ignored, while the currently used *invalid* default value of '' works. ('' is no valid language, nor is the field unset).
Comment #55
yched CreditAttribution: yched commentedI still think we should do this too.
Just, as explained in #52 / #53, I'm not sure we should include UUID fields in that mechanism.
Comment #56
fagoI agree that having a random default value as uuid field type default, doesn't make so much sense.
Yeah, that would make sense to me - so you can see the default value from looking at the BFD. The storage's create method is the last place I'd look for a default value assignment actually.
So for config entities, couldn't we do that in the Entity base classes' pre/post create method instead?
Comment #57
yched CreditAttribution: yched commentedYep, I think #56 would make sense to me too - if we do include the "for ConfigEntities, create the UUID in ConfigEntityBase::pre/postCreate()" part :-)
The current situation where:
- EntityStorageBase::create() seems to provide a generic mechanism for UUID generation,
- but you have to look close enough to understand that it's only opt-in (only if the actual storage subclass has a $this->uuidService),
- and that ContentEntityBase choses to opt-out (by not having a $this->uuidService), and instead has UUIDs generated in a whole different code flow (field default values)
feels very confusing to me :-)
Comment #58
yched CreditAttribution: yched commentedAlso, looking back at it:
feels like we should highlight a bit more the "special/magic placeholder" aspect. As is, they look like legit strings for a regular, literal default value.
Maybe we make them look weirder ?
'**RANDOM**'
,'**DATE**'
? (Views has/had something like that IIRC)Also, those magic placeholders only apply to a given field type, and would be treated as a "regular" litteral if used with another field type. 'RANDOM' feels like something you might be very tempted to use on other fields types :-)
I was thinking about using class constants defined by the individual FieldItem classes, but I guess that would prevent using them in annotations for "all fields of this type have a dynamic default value" (is that really useful though ?) Or maybe just stick to
'**UUID**'
for the "generate uuid" placeholder ?Comment #59
tstoecklerRe #58: Can't we just make them constants somewhere? (Then I don't care very much about the actual values.) But in my world a kitten dies everytime I see "***LANGUAGE_language_content***"...
Also, with this patch, there's no reason for
ChangedItem
to extendCreatedItem
any longer, is there?Comment #60
yched CreditAttribution: yched commented@tstoeckler - from #58
I guess this depends on whether we need to support "the field type enforces a 'default default value' in its field type definition" :-)
Comment #61
yched CreditAttribution: yched commentedOh, right, the whole point of CreatedItem as a separate field type is that it encapsulates the "default value is NOW" behavior in its field type definition. So it requires the "magic placeholders" to be something you can put in an @Annotation.
Never been fully convinced about that :-), and given the discussion in #56 / #57 about using
$field['uuid'] = new BFD('uuid')->setDefaultValue('RANDOM');
for uuid generation, IMO doing the same for 'created' field (regular timestamp field with an explicit default value of 'NOW' in the BFD) would be consistent..
Comment #64
yched CreditAttribution: yched commentedCrosslinking to #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state, that is discussing uuid generation
Comment #65
jhedstromIs there anything left that can be done in 8.x here?
Comment #66
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
Comment #70
andypostThat needs work for issue summary
Comment #79
quietone CreditAttribution: quietone at PreviousNext commenteddarvanen and I discussed this in #bugsmash. We think this is a task instead of a bug so changing category. And it certainly needs an Issue Summary update. I added the standard template to start that process off.
Comment #80
quietone CreditAttribution: quietone as a volunteer commentedReally change to a task