Problem/Motivation
Currently the timestamp base field on Log entities is not required, but it is always populated with a default value if it is left blank.
Technically speaking, all logs MUST have a timestamp. It is a critical piece of their data architecture.
This came to my attention recently when users of farmOS pointed out that timestamp is not marked as required in the JSON Schema.
I tried to dig back in time to figure out why they weren't required, and I traced it to this issue: #3176676: Improvements on log storage and UI
In that issue, we (at my direction, so I take the blame haha), removed setRequired(TRUE) because I said: "Make timestamp and state fields not required in form (timestamp will be autofilled)". We later rolled back the state field change.
Seeing that, I was worried that we were a bit stuck, because it might be a breaking change to make it required now. My thought was that if there were any users would pushed logs via API without timestamp (relying on the auto-populated timestamp), that code would break because it would require a timestamp be set. However, in testing this, I found that the default value gets populated *before* the check is performed, so it is *still* possible to submit a log without a timestamp, and it works exactly the same as before.
So as far as I can tell, this change would not break anything.
Proposed resolution
Add setRequired(TRUE) on timestamp base field.
Remaining tasks
Create MR.
User interface changes
Timestamp will be marked as required in log form UI.
API changes
Timestamp will be marked as required in JSON Schema when using JSON:API Schema module.
Data model changes
Timestamp will be required at the entity definition level.
Issue fork log-3396419
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
m.stentaLet's see if this breaks any tests...
Comment #4
paul121 commentedThis LGTM. Trying to think where else this change could cause issues but not sure. Glad this doesn't break JSON:API!
Comment #6
m.stentaI'm going to merge this and tag 2.2.0. It probably doesn't need a minor release, but I felt that was a bit safer than a patch release, just in case we find that this causes any issues.
Comment #8
paul121 commentedUnfortunately this has caused a breaking change starting with Drupal 10.2. The log entity form no longer sets the default value for the timestamp field and because the field is required, a value must be entered in order to submit the form. If this field was not required then I believe the current time would continue be used, although it wouldn't be shown as the default value in the entity form.
Previously the
datetime_timestampwidget would always use default value set for the field at$items[$delta]->valuebut now this widget only uses the default value if the parent entity (the log) is not new. This was changed in #2791693: Remove sample date from date field error message and title attribute: https://git.drupalcode.org/project/drupal/-/commit/5404ebfb11155b53dee0e...Just re-opening this issue to flag this... we may want to open a new issue in Log or contribute to Drupal core to fix. I left a comment in the relevant issue for now: https://www.drupal.org/project/drupal/issues/2791693#comment-15399165
Comment #9
paul121 commentedI've opened a new issue in Drupal core: #3414883: [regression] datetime_timestamp widget does not use default field value
Comment #10
m.stentaThanks @paul121! Reading your comments in #2791693: Remove sample date from date field error message and title attribute and #3414883: [regression] datetime_timestamp widget does not use default field value, it does indeed sound like a core issue, not a Log module issue (although we were the lucky ones to discover it with this change). I think we can leave this closed, patch core in farmOS (until they merge your fix), and refer other Log users there if anyone experiences it. Thanks for identifying this and for making that quick core patch!