Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Berdir’s picture

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

Status: Active » Needs review
FileSize
22.57 KB
2.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’s picture

#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

You're right, this is much simpler.

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

Wim Leers’s picture

Issue tags: -Needs tests

👌

Berdir’s picture

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

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

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

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

Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative, +Needs issue summary update
FileSize
904 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

Status: Needs work » Needs review
FileSize
904 bytes
mstrelan’s picture

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.