Problem/Motivation

See #2824851-205: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior + #2824851-206: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior + #2824851-207: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

Details to follow.

Proposed resolution

Ensure this doesn't result in a fatal error.

Remaining tasks

  1. Patch that adds failing test.
  2. Make test pass by adding a solution.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#26 2930968-26.patch904 bytesAnkit.Gupta
#24 2930968-24.patch904 bytesmstrelan
#6 2930968-6.patch903 bytescburschka
#3 2930968-2.patch2.3 KBcburschka
#3 2930968-2-crosstest-2824851-203.patch22.57 KBcburschka

Comments

Wim Leers created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I would expect that should be done on validation, might be related to #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field, if the problem indeed only happens when saving. It would be a rest-specific problem if doing that would completely break the entity, so that validation wouldn't work anymore.

cburschka’s picture

cburschka
they
commented
Status: Active » Needs review
StatusFileSize
new 2930968-2-crosstest-2824851-203.patch22.57 KB
new 2930968-2.patch2.3 KB

This needs to be tested together with #2824851-203: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, since the request will otherwise be denied without hitting the error.

(Should return an HTTP 500.)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field

I would expect that should be done on validation

+1

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#3: I think the fatal error should be reproducible even without using REST.

Something like:

  $entity = $this->entityStorage->load(…);
  $entity->set('langcode', []);
  $entity->validate();
  $entity->save();

… should trigger the same fatal error.

Such a test would not depend on #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior landing first.

cburschka’s picture

cburschka
they
commented
StatusFileSize
new 2930968-6.patch903 bytes

You're right, this is much simpler.

(Adding this to FieldTranslationSqlStorageTest, since it's vaguely related.)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs tests

👌

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

An invalid entity isn't prevented from being saved, you need to explicitly assert that there are validation errors.

The last submitted patch, 3: 2930968-2-crosstest-2824851-203.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 6: 2930968-6.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

An invalid entity isn't prevented from being saved, you need to explicitly assert that there are validation errors.

The point was to have a failing test, which we now do have. Adding validation plus explicit validation error assertion is the next step, right?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Entity Field API

Or, put differently… @Berdir wrote in #2:

I would expect that should be done on validation, might be related to #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field, if the problem indeed only happens when saving. It would be a rest-specific problem if doing that would completely break the entity, so that validation wouldn't work anymore.

#6 does prove that this is a problem only when saving, because in its backtrace, it lists:

/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:820
/var/www/html/core/lib/Drupal/Core/Entity/Entity.php:387

Both of which are save() methods.

Plus, #6 of course doesn't use the REST module at all, which means it definitely proves this is not a REST-specific problem.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2933408: Remove broken allowance of empty langcode in a PATCH request

@effulgentsia opened the related #2933408: Remove broken allowance of empty langcode in a PATCH request.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative, +Needs issue summary update
StatusFileSize
new 2930968-24.patch904 bytes

Reviewing this as part of the Bug Smash Initiative. Patch in #6 was no longer applying so I've re-rolled it. The patch still fails for me so I assume this is still an issue, didn't quite follow all the related issues. Would be good to have a concise description of the problem in the issue summary.

Status: Needs review » Needs work

The last submitted patch, 24: 2930968-24.patch, failed testing. View results

Ankit.Gupta’s picture

Ankit.Gupta commented
Status: Needs work » Needs review
StatusFileSize
new 2930968-26.patch904 bytes
mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

This is the second time I've seen Ankit.Gupta repost the exact same patch someone else posted. Unchecking the suggested credit box and setting back to NW.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.