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

Command icon 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

m.stenta created an issue. See original summary.

m.stenta’s picture

Status: Active » Needs review

Let's see if this breaks any tests...

paul121’s picture

This LGTM. Trying to think where else this change could cause issues but not sure. Glad this doesn't break JSON:API!

m.stenta’s picture

Status: Needs review » Fixed

I'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.

Status: Fixed » Closed (fixed)

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

paul121’s picture

Status: Closed (fixed) » Needs work

Unfortunately 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_timestamp widget would always use default value set for the field at $items[$delta]->value but 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

paul121’s picture

m.stenta’s picture

Status: Needs work » Fixed

Thanks @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!

Status: Fixed » Closed (fixed)

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