Core still has a couple surprising references to field_sql_storage:
- Some tests try to enable it - you'd expect this would break, but it seems to pass
- DatabaseSotrageController::onBundleRename() checks $field['storage']['type'], which doesn't exist anymore. Shows we lack some tests here.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
16.68 KB

Patch.

- There was another problem in DatabaseStorageController::onBundleRename() - the method now runs before instances are updated, so we need to get instances associated to the old bundle name.
- The test needed to fix inconsistencies in entity_test_[create|rename|delete]_bundle() as compared to entity_test_entity_bundle_info(). Hope this won't break other things :-/

yched’s picture

#1 had some tests commented out...

Status: Needs review » Needs work

The last submitted patch, field_sql_storage-2086095-2.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

#2: field_sql_storage-2086095-2.patch queued for re-testing.

jibran’s picture

Issue tags: +Field API, +Entity Field API

Tagging.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, @yched++!

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -867,22 +867,22 @@ public function onInstanceDelete(FieldInstanceInterface $instance) {
-    // We need to account for deleted or inactive fields and instances.
-    $instances = field_read_instances(array('entity_type' => $this->entityType, 'bundle' => $bundle_new), array('include_deleted' => TRUE, 'include_inactive' => TRUE));
+    // We need to account for deleted or inactive fields and instances. The
+    // method runs before the instance definitions are updated, so we need to
+    // fetch them using the old bundle name.
+    $instances = field_read_instances(array('entity_type' => $this->entityType, 'bundle' => $bundle), array('include_deleted' => TRUE, 'include_inactive' => TRUE));

Nice, that's why the call below doesn't fail ;)

No test-only patch, but I manually confirmed that the added tests fail without the fix. Changes look great, so I think this is ready to go.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.29 KB

Reroll

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies.

swentel’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
14.36 KB
Hydra’s picture

  1. +++ b/core/modules/editor/lib/Drupal/editor/Tests/EditorFileUsageTest.php
    @@ -19,7 +19,7 @@ class EditorFileUsageTest extends DrupalUnitTestBase {
    +  public static $modules = array('system', 'editor', 'editor_test', 'filter', 'node', 'entity', 'field', 'text', 'file');
    

    The module list in EditorFileUsage test has been shortened in #1605290: Enable entity render caching with cache tag support, so I removed this one from the patch.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeValidationTest.php
    @@ -19,7 +19,7 @@ class NodeValidationTest extends DrupalUnitTestBase {
    +  public static $modules = array('node', 'entity', 'field', 'text', 'filter');
    

    Same here

Rerolled the patch with the changes, so basically nothing changed and still RTBC.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API, -Entity Field API

The last submitted patch, field_sql_storage-2086095-11.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API, +Entity Field API

#11: field_sql_storage-2086095-11.patch queued for re-testing.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Should be bot fluke

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e40eb19 and pushed to 8.x. Thanks!

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