Problem/Motivation

Timestamp fields have a range constraint so that no value outside of the range of the database storage can be saved. The changed and created field types extend the timestamp field type but are missing that constraint. That means that invalid created or changed values that enter the system via a migration or JSON:API, for example, are not properly validated and may cause exceptions when being saved.

Steps to reproduce

$node = Node::create([
  'title' => 'Back to the future',
  'type' => 'article',
  // This is not a 32-bit integer.
  'created' => 2147483649
]);
// This yields 0, thus no violation is recorded.
$node->validate()->count();
// Brace for SQL exception.
$node->save();

Proposed resolution

Copy the constraints over from TimestampItem to CreatedItem and ChangedItem.

Remaining tasks

User interface changes

-

Introduced terminology

-

API changes

-

Data model changes

-

Release notes snippet

-

CommentFileSizeAuthor
#12 3521088-nr-bot_m1di15cj.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3521088

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:

About issue forks

Comments

tstoeckler created an issue. See original summary.

annmarysruthy’s picture

annmarysruthy commented
Assigned: Unassigned » annmarysruthy

annmarysruthy opened merge request !11970

annmarysruthy’s picture

annmarysruthy commented
Assigned: annmarysruthy » Unassigned
Status: Active » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for picking this up @annmarysruthy!

Looks perfect.

This will need some test coverage to go in, though. I looked around a bit and found \Drupal\KernelTests\Core\Entity\EntityValidationTest::checkValidation. I think it would make sense to add some checks to that as that already tests various field-type-specific constraints. All the tested entity types should also have a created field, so just adding a test for that should work fine. And for the changed field you can check if the entity type implements EntityChangedInterface as only a couple entity types have that.

Marking needs work for that.

prabha1997’s picture

prabha1997 commented
Assigned: Unassigned » prabha1997

I am working on this issue

prabha1997’s picture

prabha1997 commented

I tried to add validation for the ChangedItem field as shown below, but I am not getting the expected validation failure for the invalid changed timestamp.

 if ($entity instanceof \Drupal\Core\Entity\EntityChangedInterface && $entity->hasField('changed')) {
      // This entity has a 'changed' field. You can now safely test it.
      $test_entity = clone $entity;
      $test_entity->set('changed', -2147483649);  // Invalid timestamp (below min range)
      // Validate the entity.
      $violations = $test_entity->validate();
      $this->assertEquals(1, $violations->count(), 'Validation failed for invalid changed timestamp.');
      $this->assertEquals('The changed timestamp is out of the valid range.', $violations[0]->getMessage());

      // Valid 'changed' timestamp.
      $test_entity->set('changed', 1234567890);  // Valid timestamp
      $violations = $test_entity->validate();
      $this->assertEquals(0, $violations->count(), 'Valid changed timestamp passed validation.');
    }
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Nice work with the test coverage.

Ahh, how annoying. I think that is because one of the test entities uses the changed_test item (i.e. \Drupal\entity_test\Plugin\Field\FieldType\ChangedTestItem) instead of the regular changed. I had missed that initially when looking into this. I guess we could also add the constraint to the test field type one, but that then kind of feels like testing the test coverage. Alternatively we could check that $entity->get('changed')->getFieldDefinition()->getType() is in fact changed (and not changed_test). I guess I like the latter solution a bit better, but also open to what you think.

prabha1997’s picture

prabha1997 commented

I've updated the test to include a check for the actual field type, so it now only runs if the changed field uses the real 'changed' type (not the changed_test).

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Thanks for sticking with this @prabha1997!

I tried it out locally and I guess my suggestion above was not really great. Turns out the inside of the if is reached. I had seen that the entity_test_constraints entity type has an actual changed field, but unfortunately that is not one of the entity types used in the test. (See \Drupal\entity_test\EntityTestHelper::getEntityTypes().) All of this is a bit of a mess, but not one we will have to fix here, I hope. So I looked into how to get an actual changed field into the entity types being tested without rewriting the entire test and found that the test entity types support adding base field definitions via state. I wasn't sure whether that actually worked as intended so I tried it out to be sure. And since I already had it then I went ahead and pushed it to the branch, I hope that was OK.

Maybe you can take a look and see if you agree with those changes, thanks!

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ran the test only job here https://git.drupalcode.org/issue/drupal-3521088/-/jobs/5700854 and got 

1) Drupal\KernelTests\Core\Entity\EntityValidationTest::testValidation
Validation failed for invalid created timestamp.
Failed asserting that 0 matches expected 1.
/builds/issue/drupal-3521088/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php:217
/builds/issue/drupal-3521088/core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php:140
FAILURES!
Tests: 3, Assertions: 103, Failures: 1.
Exiting with EXIT_CODE=1

The use 2147483648 makes sense as mentioned it's what Timestamp uses.

So no objections.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3521088-nr-bot_m1di15cj.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Reviewed & tested by the community

Rebased (and squashed). Back to RTBC per #11

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Assigned: prabha1997 » Unassigned

Triaging the RTBC queue.

All questions are answered here and credit is updated.

I did think that the wording is the comments in the test could be improved and simplified a bit. Plus one was not wrapped correctly. Also, there were 2 @see lines in the method. I thought one was not necessary and the other should be in the method doc block. Given those changes are only to comments and did not change the intent I am leaving at RTBC.

catch made their first commit to this issue’s fork.

catch closed merge request !11970

  • catch committed 043582c0 on 11.x 
    Issue #3521088 by tstoeckler, annmarysruthy, prabha1997, quietone:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This looks good, I originally mis-read #15 as needing fixes but then saw the commit to improve the comments and agreed with leaving RTBC. Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Wow! I just ran into this while working on #3533675: Avoid suggesting UNIX timestamp integers for `type: integer` props! So nice to see this fixed already 🤩