Updated: Comment #N
Problem/Motivation
Various content entity types have changed and/or created timestamps and they all have to keep them updated manually and do so in different ways. We're also adding it to more entity types.
Additionally, there's no a special validation constraint for changed fields that is used to detect conflicting changes, something that used to be node specific.
Proposed resolution
The patch adds three new field types/FieldItem classes:
- TimestampItem, stored as an integer, the only difference is that the Timestamp class does implement DateTimeInterface, so you can directly get a date object from it with $node->changed->get('value')->getDate(), we currently don't use that though.
- CreatedItem, a subclass of that, does just one additional thing, setting the default value to the current timestamp when being created.
- ChangedItem, a subclass of CreatedItem, automatically updates itself when being saved.
The result is that content entities can simply define their changed/created fields using the correct type and then they just work.
Remaining tasks
User interface changes
-
API changes
Only additions, the new field types can be used but doing it manually is still possible and unchanged
Original report by @fago
As noted in #2110345: Simplify validation constraint implementations for fields the current EntityChangedConstraint is a bit complex and could be simplified. Imo, we do not even need a special constraint for that but should have a non-configurable entity-field (similar to UUID field) that takes care of all the necessary logic around the field and can configure a regular date comparison constraint instead of an EntityChangedConstraint, see #2110345-4: Simplify validation constraint implementations for fields:
So the field type would
a) care about setting/updating the value as appropriately
b) implement getConstraint() to return a constraint on the field, comparing it with last changed that of the entity.
I'm not sure we've the necessary constraint for date comparisons in core though, but it should be in symfony validator.
Comment | File | Size | Author |
---|---|---|---|
#60 | changed-field-type-2145103-60.patch | 23.29 KB | Berdir |
#57 | changed-field-type-2145103-57-interdiff.txt | 718 bytes | Berdir |
#57 | changed-field-type-2145103-57.patch | 23.08 KB | Berdir |
#55 | changed-field-type-2145103-55-interdiff.txt | 1.04 KB | Berdir |
#55 | changed-field-type-2145103-55.patch | 23.08 KB | Berdir |
Comments
Comment #1
BerdirThis makes a lot of sense, validation actually just being one of multiple advantages.
It also allows to standardize the update on preSave() logic, helps figuring out which is the changed field name (if you e.g. need it for an EFQ query) and possibly more.
Not sure I fully understand the validation changes, but it doesn't even matter that much how it's done, developers just need to set the field type and then it will work, if it's a custom constraint or not is kind of irrelevant for the API.
Comment #2
BerdirStarted with this, added a timestamp, created and changed field. Did not yet change the validation constraint, just moved it into the class.
Comment #5
benjy CreditAttribution: benjy commentedWould be good to get this in before: #2074475: Rename {file_managed}.timestamp to 'changed' and add a created timestamp which is a beta blocker.
Comment #6
BerdirApart from the fact that I don't really see why that is a beta blocker, it doesn't really matter in which order it goes in. It would save a few lines in the other issue, yes, but either issue can easily be re-rolled if the other one gets committed.
Comment #7
fagoFixed a few issues, moved constraint to the type definition and improved descriptions a bit.
Also, I'm wondering whether we should prefix the new field types with time, so that all time* field types are somewhat grouped together? -> time_created, time_changed, timestamp ?
Comment #8
benjy CreditAttribution: benjy commentedUsually i'd agree that a prefix is clearer however it's a pretty common and understand to have changed/created fields so i'm not sure it matters much either way.
Out of interest, could you explain what this does? I can't find much/any docs on yml, complex data and constraints. Eg how did you know to use the EntityChanged key?
Comment #11
BerdirDidn't fix the empty array thing in annotations?
#1848570: Upgrade to Doctrine\Common 2.4
So those overrides shouldn't be necessary anymore...
Comment #12
fagoEntityChanged is the plugin name of the respective constraint, look for a class named like it. Then, documentation on the annotation docblock is usually best found by looking up the annotation class, i.e. FieldType in this case it's parent class DataType.
oh nice. However it's in use for the Email type as well, so we need to fix it there as well then.
Comment #13
BerdirThis should fix most tests, also removing the empty array hack.
Comment #14
tstoecklerI'm not keen enough to RTBC myself, but I couldn't find anything to complain about.
Also, this patch is **awesome**. Really, really awesome!
Comment #15
BerdirThis is actually not supposed to be green yet :)
See #2005716-129: Promote EntityType to a domain object, there are tests that alter the node entity class to prevent changed from being overridden, so this shouldn't pass, strange. Maybe the test run was so slow that it didn't matter.
There might also be some other changed related tests, that could be affected by this, I know there's one that changes the changed date in a presave hook.
Comment #16
Berdir13: changed-field-type-2145103-13.patch queued for re-testing.
Comment #17
BerdirNoticed that the change in ConstraintManager should be unnecessary now and found another snippet that can be removed.
@fago initially said that he wants to to the validation part differently, is that still on the table/necessary or should we consider to do that later, as it should now be fairly transparent to whoever's using it?
Comment #18
Wim LeersStrange newline?
Is it okay to use
t()
?Why does this extend
CreatedItem
and notTimestampItem
? Because we assume that you cannot want aChangedItem
without also having aCreatedItem
? Is that even true? Why does aChangedItem
need to useCreatedItem::applyDefaultValue()
, which it does due to inheritance?Nice :) I think I probably need to apply this change everywhere in
edit.module
?Comment #20
Berdir17: changed-field-type-2145103-17.patch queued for re-testing.
Comment #22
fagoYeah, I'd be happy with keeping the existing approach for now - the important part is that people won't have to use the constraint manually more, so the constraint used becomes an unimportant implementation detail.
Changing that would be a nice to have, i.e. I'd suggest opening an issue for later. We've got more important stuff to work on right now.
Comment #23
fagoad #18,3: Yeah, semantically the inheritance doesn't fit really - it's just for code re-use. Maybe we should better just copy that part over as we cannot go with traits yet?
Comment #24
fagoComment #25
das-peter CreditAttribution: das-peter commentedre-rolling
Comment #26
das-peter CreditAttribution: das-peter commentedRegarding #18
TimestampItem::getPropertyDefinitions()
t()
seems to be fine. It's used in all other implementations too.As far as I understand that doesn't imply that there's an created field, however the
applyDefaultValue()
is inherited. And I'm not really sure if this is needed / ok like that. Wouldn't that initialization prevent us from recognize if there was a change already or not?The aggregator tests failed because the timestamp and not the created item was used. Adjusted now.
Re-roll was necessary due the "newly" introduced
schema
method inFieldItemInterface
.Comment #27
das-peter CreditAttribution: das-peter commentedUpdated patch to cover file entity as well.
Additional cleanup change for file entity to remove legacy handling of language field.
Comment #28
BerdirFor the record, it's like that in pretty much all item classes right now.
Not relevant as it will go away soon anyway.
I don't really understand this. It would only be relevant for an entity that is still new, and then changed is the same as created, aka now. Without applyDefaultValue(), it would be NULL. I think I explicitly extended it from CreatedItem because there are some tests that test that behavior.
Comment #30
BerdirPatch passed, just the incorrectly named interdiff caused it to be set to needs work.
Comment #33
das-peter CreditAttribution: das-peter commented*grml* sorry for the noise.
I'm confused now - one of the last test said the patch failed too.
Re-adding to have a proper test - no difference to previous patch.
Comment #35
Berdir33: changed-field-type-2145103-33.patch queued for re-testing.
Comment #37
Berdir33: changed-field-type-2145103-33.patch queued for re-testing.
Comment #38
amateescu CreditAttribution: amateescu commentedThis patch looks great! Nice cleanups and fixed @todos :)
Comment #39
dawehnerIs it just me that the description explains things way better than the general class documentation?
Just figured it out: annotations recently got empty array support.
Semantically this feels a bit odd but let's be pragmatic here.
That is way better to read now, thank you!
Comment #40
alexpottchanged-field-type-2145103-33.patch no longer applies.
Comment #41
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #42
alexpottComment #43
Berdir41: changed-field-type-2145103-41.patch queued for re-testing.
Comment #45
BerdirAnother re-roll, only conflicted in use statements.
Comment #47
Berdir45: changed-field-type-2145103-45.patch queued for re-testing.
Comment #49
Berdir45: changed-field-type-2145103-45.patch queued for re-testing.
Comment #50
jibran45: changed-field-type-2145103-45.patch queued for re-testing.
Comment #52
BerdirRe-roll, who wants to set this to RTBC? :)
Comment #53
benjy CreditAttribution: benjy commentedComment #55
BerdirRe-roll.
Comment #57
BerdirUpdated the code to fix those test fails.
Comment #58
tstoecklerBack to RTBC.
Comment #59
alexpottchanged-field-type-2145103-57.patch no longer applies.
Comment #60
BerdirRe-roll. Conflicted with the changed baseFieldDefinitions() method, nothing in the patch changed.
Comment #61
Solthun CreditAttribution: Solthun commentedComment #62
aspilicious CreditAttribution: aspilicious commentedLooking good
Comment #63
webchickOverall this looks like really nice clean-up, and yay consistency for frequently-used fields.
However...
This is a "pre-existing condition," but... ouch. :( I literally have no idea how to translate that into English. Looks like jibran had trouble up above, too...
I'm going to make a major (for now) beta target to clean up that DX, because...
...this is just silly. I see that it saves us a 3-line hack in TypedDataManager, but it really makes the DX totally non-intuitive. (And easy to mis-type, too... accidentally leaving off a }, typing a ] instead, etc.)
Both of those are issues exposed with the patch, not caused by it though, so no reason to hold this up.
In terms of change notice, Berdir said it didn't really make sense to make one specifically about this, since the code it's replacing was embedded in node_save(). He's going to update https://drupal.org/node/1806650 and https://drupal.org/node/2031221 instead to note this for people upgrading from 7 to 8, since those are the main places they'll look.
Therefore, committed and pushed to 8.x. Thanks!