Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchemaConverter::convertToRevisionable is making a call to \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface::getLastInstalledFieldStorageDefinitions, but a typo introduced in #3004642: Deprecate SqlContentEntityStorageSchemaConverter in favor of the new API provided by EntityTypeListenerInterface::onFieldableEntityTypeUpdate() (committed 13 Feb 2019) hard coded the entity_test_update entity type instead of the entity type being updated.
*/
public function convertToRevisionable(array &$sandbox, array $fields_to_update = []) {
/** @var \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type */
$this->entityTypeManager->useCaches(FALSE);
$entity_type = $this->entityTypeManager->getDefinition($this->entityTypeId);
/** @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface $last_installed_schema_repository */
$last_installed_schema_repository = \Drupal::service('entity.last_installed_schema.repository');
$field_storage_definitions = $last_installed_schema_repository->getLastInstalledFieldStorageDefinitions('entity_test_update');
Understandably SqlContentEntityStorageSchemaConverter was deprecated in 8.7, so usage in core has been eliminated. Existing code, especially before 8.7.x, and documentation refer to the convertToRevisionable method.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3052140-13.patch | 4.45 KB | amateescu |
Comments
Comment #2
dpiComment #3
dpiComment #4
jibranYeah, it happens.
Comment #5
dpiComment #6
xjmlol whoops.
This should probably have a test, which would clearly need to use an entity type other than a test entity type. ;) Assuming there are already tests that we can just add to?
Comment #7
xjmComment #8
plachSqlContentEntityStorageSchemaConverteris deprecated and should be no longer used, tests have been migrated over, which might explain why this slipped through.Comment #9
plachI'm not sure about this needing tests TBH.
Comment #10
xjmI guess if it's a deprecated API there's a debate to be had about whether it needs tests (and I can probably predict which committers would take which positions on the matter). However, can we start by looking into how difficult test coverage would be first? If it's trivial there's no reason not to add it.
A workaround we could document in the IS is using the replacement API.
Comment #11
amateescu CreditAttribution: amateescu commentedIMO, writing test coverage for this is not trivial at all because we have only one entity type which is suited for update path tests, and.. you guessed it, it's the one from this patch:
entity_test_update:)Comment #12
alexpottI'd be +1 to committing this without tests because it's an obvious error and just a mistake and it is something we broke in 8.7.x that was working in 8.6.x.
Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI found another entity type which could be used for testing this:
entity_test_new, so here's a quick test.Comment #15
jibranThanks, this is ready now.
Comment #16
alexpottThanks @dpi for filing the issue and @amateescu for writing tests and @xjm for asking for them.
Committed and pushed 20762532ac to 8.8.x and 2cdefde175 to 8.7.x. Thanks!
Backported to 8.7.x as this is an update critical.