testbot playground for #1497374: Switch from Field-based storage to Entity-based storage

If you want to help, please

  1. Pick a failing test from the latest patch
  2. File an issue on https://drupal.org/sandbox/chx/1857558 with the classname as the node title
  3. Fix test
  4. File patch
  5. Rinse and repeat

There are no systematic issues we are aware of, it's the tests themselves that are broken and need to be fixed one by one. Ping chx on IRC for help as necessary.

CommentFileSizeAuthor
#132 interdiff_123_124.txt126.06 KBchx
#131 field_storage-testbot-2047777-130.patch603.97 KByched
#128 field_storage-testbot-2047777-127.patch603.91 KByched
#126 field_storage-testbot-2047777-126.patch598.55 KByched
#124 field_storage-testbot-2047777-124.patch598.45 KByched
#123 2047777_122.patch510.95 KBchx
#122 2047777_122.patch0 byteschx
#120 field_storage-testbot-2047777-120.patch604.36 KByched
#118 field_storage-testbot-2047777-118.patch601.9 KByched
#117 field_storage-testbot-2047777-117.patch601.09 KByched
#116 field_storage-testbot-2047777-116.patch573.76 KByched
#115 field_storage-testbot-2047777-115.patch574.25 KByched
#113 field_storage-testbot-2047777-113.patch573.34 KByched
#110 field_storage-testbot-2047777-110.patch496.46 KByched
#110 interdiff.txt16.21 KByched
#109 field_storage-testbot-2047777-109.patch496.15 KBswentel
#109 interdiff.txt2.91 KBswentel
#107 field_storage-testbot-2047777-107.patch496.33 KBswentel
#107 interdiff.txt15.88 KBswentel
#105 field_storage-testbot-2047777-105.patch510.97 KBswentel
#105 interdiff.txt15.42 KBswentel
#102 field_storage-testbot-2047777-102.patch492.95 KBswentel
#102 interdiff.txt30.17 KBswentel
#100 field_storage-testbot-2047777-99.patch486.88 KBswentel
#97 field_storage-testbot-2047777-97.patch485.94 KBswentel
#93 field_storage-testbot-2047777-93.patch485.84 KBswentel
#93 interdiff-on-interdiff.txt5.02 KBswentel
#89 field_storage-testbot-2047777-89.patch485.08 KByched
#88 field_storage-testbot-2047777-88.patch486.62 KBswentel
#88 interdiff.txt16.64 KBswentel
#86 field_storage-testbot-2047777-86.patch485.69 KBswentel
#84 field_storage-testbot-2047777-84.patch484.4 KBswentel
#84 interdiff.txt11.31 KBswentel
#82 field_storage-testbot-2047777-82.patch484.29 KBswentel
#80 field_storage-testbot-2047777-80.patch484.88 KByched
#78 field_storage-testbot-2047777-78.patch483.37 KBswentel
#78 interdiff.txt9.48 KBswentel
#77 field_storage-testbot-2047777-77.patch483.06 KByched
#77 interdiff.txt23.11 KByched
#72 field_storage-testbot-2047777-72.patch475.21 KByched
#72 interdiff.txt27.82 KByched
#71 field_storage-testbot-2047777-71.patch486.67 KByched
#71 interdiff.txt5.45 KByched
#70 field_storage-testbot-2047777-70.patch489.25 KByched
#70 interdiff.txt6.8 KByched
#69 field_storage-testbot-2047777-69.patch486.84 KByched
#69 interdiff.txt2.94 KByched
#66 interdiff.txt9.18 KByched
#62 field_storage-testbot-2047777-62.patch488.94 KByched
#62 interdiff.txt527 bytesyched
#60 field_storage-testbot-2047777-60.patch488.89 KByched
#60 interdiff.txt7.08 KByched
#59 2047777_59.patch493.95 KBchx
#57 2047777_57.patch495.36 KBchx
#55 2047777_55.patch495.04 KBchx
#38 2047777_36.patch471.7 KBchx
#34 field_storage-testbot-2047777-34.patch462.15 KByched
#32 field_storage-testbot-2047777-32.patch462.14 KByched
#32 interdiff.txt22.83 KByched
#29 field_storage-testbot-2047777-29.patch455.3 KByched
#29 interdiff.txt2.65 KByched
#27 field_storage-testbot-2047777-27.patch454.11 KByched
#27 interdiff.txt8.34 KByched
#24 field_storage-testbot-2047777-24.patch450.9 KBamateescu
#24 interdiff.txt16.27 KBamateescu
#21 field_storage-testbot-2047777-21.patch440.77 KByched
#17 field_storage-testbot-2047777-17.patch446.28 KByched
#17 interdiff.txt18.73 KByched
#15 field_storage-testbot-2047777-15.patch437.28 KByched
#15 interdiff.txt12.36 KByched
#13 field_storage-testbot-2047777-13.patch428.4 KByched
#13 interdiff.txt1.54 KByched
#11 field_storage-testbot-2047777-11.patch428.39 KByched
#11 interdiff.txt2.16 KByched
#9 field_storage-testbot-2047777-9.patch426.79 KByched
#9 interdiff.txt2.04 KByched
#6 field_storage-testbot-2047777-5.patch425.62 KByched
#4 field_storage-testbot-2047777-4.patch425.62 KByched
#4 interdiff.txt32.43 KByched
#1 field_storage-testbot-2047777-1.patch411.71 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
411.71 KB

Current state of the sandbox.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-1.patch, failed testing.

yched’s picture

Investigating those numerous "FieldException: Attempt to create an instance of unknown, disabled, or deleted field XXX in Drupal\field\Plugin\Core\Entity\FieldInstance->__construct()" fails.

yched’s picture

Status: Needs work » Needs review
FileSize
32.43 KB
425.62 KB

A couple fixes.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-4.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
425.62 KB

Better with a reroll.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-5.patch, failed testing.

yched’s picture

Hmm, that's *more* fails :-(.
I can only assume that less early fatals means more asserts get a chance to fail (there is also more passes...)

yched’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
426.79 KB

I can only assume that less early fatals means more asserts get a chance to fail (there is also more passes...)

That was the case. All upgrade tests now failed a bit later, with 2 fails and 4 warnings instead 1 fail and 1 warning in previous patch, that pretty much accounts for the difference...

Attached patch should fix upgrades, and a bunch of other fatals.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-9.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
428.39 KB

Other attempt at fixing the upgrade path.

taxonomy_update_8007() accounts for a column being renamed (tid -> target_id)
- It needs to run *before* field_update_8006(), because that one only moves data columns that have the expected column name as per the current field schema (target_id)
It would probably be best if field_update_8006() just moved all the columns it found in the original tables (removing the need to rely on the runtime field schema / columns), but I'm not sure there's a way to do that. @chx ?
- It then needs to hardcode the old table names, and cannot use DatabaseStorageController::_fieldTableName() since that one will return the new table name, which will only exist after field_update_8006().

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-11.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
428.4 KB

Trying something.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-13.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
12.36 KB
437.28 KB

Drats. Re-trying after reroll.
Patch additionally contains a couple more fixes.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-15.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
18.73 KB
446.28 KB

OK, reverted my attempt on the query in field_update_8006() (see interdiff from #13)
Just fixed the method name (->fromQuery() should be from()), and db_select()->fields() needs a table alias.

Plus a couple more fixes here and there.

yched’s picture

Identified, but not fixed yet:
- user_update_8012() needs some special care, it tries to write to the old data tables. See fails in UserPictureUpgradePathTest
- Tests having errors like "Attempt to create an instance of unknown, disabled, or deleted field %foo" in FieldInstance->__construct() probably come from tests that try to create two instances on two different entity types, confident that the field exists - works in HEAD, but now they need to create new fields on each entity type... I fixed one in FieldAttachOtherTest
- we do have errors caused by table names getting too long...
I fixed a couple by reducing random strings / field names, but we'll need a more general fix.
- Help welcome on tests for which the qa.d.o only reports "The test did not complete due to a fatal error"

I'm done for today. Help moving this forward is welcome :-)

yched’s picture

Regarding the table names: I'm not sure how we're going to escape including an auto truncating mechanism in DSC::_fieldTableName(). That would mean storing and fetching a map in state()...

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-17.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
440.77 KB

Reroll after #2045919: Replace all module_implements() deprecated function calls..

Also - I won't be able to handle all the test fails all by myself, need help here :-)

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-21.patch, failed testing.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

I will throw myself into the fray tomorrow.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.27 KB
450.9 KB

Rerolled after #2017207: [meta] Complete conversion of users to Entity Field API and fixed a lot of tests. Let's see what's left :)

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-24.patch, failed testing.

yched’s picture

w000t !

yched’s picture

Status: Needs work » Needs review
FileSize
8.34 KB
454.11 KB

Should fix a couple more.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-27.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
455.3 KB

Yep, silly me.

In addition, should hopefully fix OptionsWidgetsTest.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-29.patch, failed testing.

yched’s picture

Pushed fixes for:
EntityQueryTest
ContentTranslationSyncImageTest
NormalizeTest
NodeAccess*Test
EntityResolverTest
EntityTranslationTest

The remaining fails are in Views and upgrade path.
Will try to look into views before posting an updated patch.

yched’s picture

Status: Needs work » Needs review
FileSize
22.83 KB
462.14 KB

Should fix at least some views fails.

I'd really welcome help on the upgrade fails, running them locally makes my poor crappy laptop crawl to a corner and die :-p

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-32.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
462.15 KB

merged HEAD

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-34.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Here's an attempt at the field upgrade. It introduces a new update field pseud-hook that needs to be documented.

plach’s picture

@chx:

No patch :)

chx’s picture

FileSize
471.7 KB

Status: Needs review » Needs work

The last submitted patch, 2047777_36.patch, failed testing.

yched’s picture

FYI - pushed:
- Merged HEAD
- re-fix DenormalizeTest after #1972116: Add REST test coverage for nodes.

Does not deserve a new patch for now...

amateescu’s picture

Pushed fixes for all Views tests. We're getting very close :)

yched’s picture

   // Convert user picture to field after the fields and instances are converted
   // to ConfigEntities.
   $dependencies['user'][8011] = array(
-    'field' => 8003,
+    'field' => 8006,
   );

The comment would need to be updated accordingly.
Also, that's not the problem here - user_8011, as currently coded, puts data in the old field tables.
Either it stays this way and needs to run *before* field_8006, or it stays running after but then has to be modified to put data in the new tables.
The latter seems preferable ?

-      'indexes' => $record['data']['indexes'] ?: array(),
+      // Custom indexes are not upgraded.
+      'indexes' => array(),

Why is that ?

amateescu’s picture

Fixed Drupal\file\Tests\FileListingTest and Drupal\system\Tests\System\PasswordHashingTest.

We're down to only two tests:
- Drupal\system\Tests\Upgrade\ModulesDisabledUpgradePathTest
- Drupal\system\Tests\Upgrade\UserPictureUpgradePathTest

@chx, your turn :)

chx’s picture

Because... you can't upgrade custom indexes, not easily, not with the column names changing. In case, the table creation simply fails if we copy over the fid index... we can add another hook if you want. Or we can say that people with custom data need to write custom upgrades.

yched’s picture

Hm. Then I'm afraid it's more serious than that.

We're not supposed to rely on plugins within updates - #2055605: Disallow use of plugins during update. That means, no asking the "field type" plugin for the schema() of a given field.
#2032369: _update_8003_field_create_field() relies on field schemas never changing and plugins being available, for example, changed _update_8003_field_create_field() calls to have the caller explicitly inject a hardcoded 'schema' for the field to create.

In our case here, this means we're not supposed to call DatabaseStorageController::_fieldSqlSchema($field) in field_update_8006().
And de facto, the current field_update_8006() will fail on fields whose type is unknown.

The ideal flow for field_update_8006() our case would be:
1) Create a new table by just copying the structure of the existing one
Just take what's there and reproduce it as is, no calling runtime code to rebuild the schema we expect (which includes column names mismatches)
2) Copy the relevant values as is (all columns, no renames) from the old table to the new table
3) Drop the 'entity_type' column in the new table
Done - then each field type module can take care of renaming columns and indexes in their own separate updates, that just need to run after field_update_8006().

1) is just "CREATE TABLE new_table LIKE old_table" in MySQL - problem is, of course, I don't know if we have a cross DB / PDO API in Drupal that allows that :-/.

chx’s picture

That's the problem: we do not have a way to copy table structures in Drupal or elsewhere.

We could introduce db_copy_table('src', 'dst');.

In MySQL this is CREATE TABLE dst LIKE src

In SQLite we could read the definition of src by SELECT sql FROM sqlite_master WHERE tbl_name='src' and then massage it. We need to deal with CREATE TABLE and CREATE INDEX separately but it is definitely doable.

In PostgreSQL we can do CREATE TABLE dst (LIKE src INCLUDING ALL).

In MSSQL you are SOL.

Oracle has an SQLite-like solution SELECT dbms_metadata.get_ddl( 'TABLE', 'src' ) FROM DUAL;

yched’s picture

Hm, also, we currently don't drop the old table at the end ?

chx’s picture

We never drop tables during updates -- why would we? Let the users have their data in case we missed something. Dropping tables is easy manually.

yched’s picture

[edit: #47 was a crosspost]

Does #46 mean "let's go for it" ?

Also, one issue with our support for db_insert()->from($query) is that it assumes $query selects an explicit list of fields, the code doesn't support "INSERT INTO new SELECT * FROM old WHERE..." (that syntax is supported in MySQL, pgSQL & SQLite, though)

That one is easily worked around, though - first do a "SELECT * FROM old LIMIT 1", then grab keys in the result. Meh.

chx’s picture

Yes, #46 means I will add a copy table. Ouchie.

yched’s picture

Way cool. Thanks :-))

chx’s picture

Re #49,

     // Execute the data migration query.
    $this->connection->insert($new_table)
      ->from($select)
      ->execute();

is in core, it's certainly supported.

chx’s picture

yched’s picture

re #52 - it's not, I tested it :-)
If you look at the existing core examples for ->from($query), $query always uses an explicit list of fields.

I opened #2056363: INSERT INTO table SELECT * FROM ... not supported

chx’s picture

Status: Needs work » Needs review
FileSize
495.04 KB

Thanks for that issue, we might be green with it. This contains both the db_copy_table and that INSERT patch for testing purposes.

Of particular note is the passing of the field schema into _fieldSqlSchema to avoid an (impossible) plugin lookup. We already mandate it on a doxygen level for _update_8003_field_create_field and it is indeed specified in both usages of _update_8003_field_create_field so it was easy.

Status: Needs review » Needs work

The last submitted patch, 2047777_55.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
495.36 KB

yay last exception! Very easy fix, just a new test needing a conversion.

Status: Needs review » Needs work

The last submitted patch, 2047777_57.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
493.95 KB

Merge conflict resolved.

yched’s picture

Yay, green !

Refactored a couple stuff in field_update_8006():
- inlined a couple vars, added a couple comments
- drop the 'entity_type' column in the new migrated tables
Plus:
- moved file_8003 dependency from field_8003 (CMI) to field_8006 (table reorg)
- changed taxonomy_8007 to run after field_8006 - taxonomy_8007 and file_8003 do exactly the same thing, better if they work the same way.

Did not push yet, let's see what the bot says.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-60.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
527 bytes
488.94 KB

Doh, #60 updated the comment but forgot to actually bump the dependency number for taxonomy_update_8007...

yched’s picture

Ok, pushed #60 / #62.

yched’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FieldUpgradePathTest.phpundefined
@@ -180,10 +179,17 @@ function testFieldUpgradeToConfig() {
     // Check that field values in a pre-existing node are read correctly.
-    $body = node_load(1)->get('body');
-    $this->assertEqual($body->value, 'Some value');
-    $this->assertEqual($body->summary, 'Some summary');
-    $this->assertEqual($body->format, 'filtered_html');
+    $body = db_select(DatabaseStorageController::_fieldTableName($field_entity), 't')
+        ->fields('t')
+        ->condition('entity_id', 1)
+        ->condition('langcode', Language::LANGCODE_NOT_SPECIFIED)
+        ->orderBy('delta')
+        ->condition('deleted', 0)
+        ->execute()
+        ->fetchObject();
+    $this->assertEqual($body->body_value, 'Some value');
+    $this->assertEqual($body->body_summary, 'Some summary');
+    $this->assertEqual($body->body_format, 'filtered_html');

Looks like this change is not needed, FieldUpgradePathTest is still green if I revert it locally.
@chx, is there another reason or can we just revert it ?

jibran’s picture

Here is my not very technical review. Ignore whatever is irreverent.

+++ b/core/includes/database.incundefined
@@ -680,6 +680,18 @@ function db_rename_table($table, $new_name) {
+ * Copies the structure of a table.
+ *
+ * @param string $source
+ *   The name of the table to be copied.
+ * @param string $destination

@return missing.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.phpundefined
@@ -75,4 +75,14 @@ public function setStatus($status);
+   * Retrieves the exportable properties of the entity.
+   *
+   * These are the values that get saved into config.

I don't know but this should be on same line till 80 chars limit and no empty line needed.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -7,13 +7,15 @@
+use Drupal\field\FieldInfo;
+use Drupal\field\FieldUpdateForbiddenException;
+use Drupal\field\FieldInterface;
+use Drupal\field\FieldInstanceInterface;
 use Drupal\Core\Entity\Query\QueryInterface;
 use Drupal\Component\Uuid\Uuid;
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Core\Database\Connection;

order is wrong.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -528,42 +536,637 @@ protected function saveRevision(EntityInterface $entity) {
+   * Implements \Drupal\Core\Entity\EntityStorageControllerInterface::getQueryServiceName().

{@inheritdoc}

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -528,42 +536,637 @@ protected function saveRevision(EntityInterface $entity) {
+   * @param $schema

typehint missing

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -528,42 +536,637 @@ protected function saveRevision(EntityInterface $entity) {
+   *   The same as a hook_schema() implementation for the data and the

Can we add @see here?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -528,42 +536,637 @@ protected function saveRevision(EntityInterface $entity) {
+   * @param $field

typehint missing.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -528,42 +536,637 @@ protected function saveRevision(EntityInterface $entity) {
+      return "field_deleted_revision_" . substr(hash('sha256', $field['uuid']), 0, 10);
...
+      return "field_rev_{$field['entity_type']}__{$field['field_name']}";

Why not use rev here as well? I like rev. If it doesn't break api.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -53,8 +54,8 @@ class DatabaseStorageControllerNG extends DatabaseStorageController {
    * Overrides DatabaseStorageController::__construct().

This needs proper doc block.

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -311,9 +312,9 @@ public function validate(array $form, array &$form_state) {
+      $definitions = \Drupal::entityManager()->getFieldDefinitions($entity_type, $entity->bundle());
+      foreach (field_info_instances($entity_type, $entity->bundle()) as $field_name => $instance) {

@@ -330,7 +331,7 @@ public function validate(array $form, array &$form_state) {
+        $langcode = field_is_translatable($entity_type , field_info_field($entity_type, $field_name)) ? $entity_langcode : Language::LANGCODE_NOT_SPECIFIED;

This should be injected but probably in follow up.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -104,21 +113,45 @@ class EntityManager extends PluginManagerBase {
+  /**
+   * Add more namespaces to the entity manager.
+   *
+   * This is usually only necessary for uninstall purposes.

This should be on same line

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,6 +234,22 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+   * @param $hook

typehint missing.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,6 +234,22 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+    module_invoke_all($this->entityType . '_' . $hook, $entity);
...
+    module_invoke_all('entity_' . $hook, $entity, $this->entityType);

We should inject/pass __construct the moduleHandler or at least use Drupal::

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.phpundefined
@@ -0,0 +1,322 @@
+   * @param $entities
...
+   * @param $age

typehint missing.

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.phpundefined
@@ -0,0 +1,322 @@
+          $cid = "field:$this->entityType:$id";

Can we change this to "field:{$this->entityType}:$id"

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.phpundefined
@@ -0,0 +1,322 @@
+   * @return
+   *   Default values (if any) will be added to the $entity parameter for fields

Do we need @return if yes then typehint missing.

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.phpundefined
@@ -0,0 +1,322 @@
+   * Saves field data for an existing entity.
+   *
+   * It should be enough to override doFieldUpdate() instead of this method.
...
+   * Deletes field data for an existing entity. This deletes all revisions of
+   * field data for the entity.
+   *
+   * It should be enough to override doFieldDelete() instead of this method.
...
+   * Delete field data for a single revision of an existing entity. The passed
+   * entity must have a revision ID attribute.
+   *
+   * It should be enough to override doFieldRevisionDelete() instead of this

Should be on same line.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.phpundefined
@@ -108,7 +109,7 @@ function addField($field, $type, $langcode) {
+        $field = field_info_field($entity_type, $specifier);

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -181,13 +181,13 @@ function custom_block_entity_bundle_info() {
+  $field = field_info_field('custom_block', 'block_body');

+++ b/core/modules/comment/comment.moduleundefined
@@ -322,11 +322,11 @@ function comment_node_type_delete($info) {
+  if (!field_read_field('comment', 'comment_body', array('include_inactive' => TRUE))) {

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
@@ -45,7 +45,7 @@ function testCommentDefaultFields() {
+    $field = field_info_field('comment', 'comment_body');

@@ -54,7 +54,7 @@ function testCommentDefaultFields() {
+    $field = field_info_field('comment', 'comment_body');

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLanguageTest.phpundefined
@@ -71,7 +71,7 @@ function setUp() {
+    $field = field_info_field('comment', 'comment_body');

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTranslationUITest.phpundefined
@@ -63,7 +63,7 @@ protected function getTranslatorPermissions() {
+    $field = field_info_field('comment', 'comment_body');

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentUninstallTest.phpundefined
@@ -42,7 +42,7 @@ protected function setUp() {
+    $field = field_info_field('comment', 'comment_body');

@@ -50,7 +50,7 @@ function testCommentUninstallWithField() {
+    $field = field_info_field('comment', 'comment_body');

@@ -60,12 +60,12 @@ function testCommentUninstallWithField() {
+    $field = field_info_field('comment', 'comment_body');
...
+    $field = field_info_field('comment', 'comment_body');

Can we use Drupal:: and not use deprecated functions anymore?

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Form/TranslatableForm.phpundefined
@@ -74,9 +75,9 @@ public function getCancelPath() {
+  public function buildForm(array $form, array &$form_state, $entity_type = NULL, $field_name = NULL) {

Doc block above this function needs update.

+++ b/core/modules/field/field.crud.incundefined
@@ -45,8 +45,8 @@
+function field_read_field($entity_type, $field_name, $include_additional = array()) {

Doc block needs update.

+++ b/core/modules/field/field.installundefined
@@ -98,6 +94,68 @@ function _update_8003_field_create_instance(array $field_config, array &$instanc
+ *

Extra white space.

+++ b/core/modules/field/field.installundefined
@@ -98,6 +94,68 @@ function _update_8003_field_create_instance(array $field_config, array &$instanc
+ * @param string $entity_type
+ * @param string $bundle
+ * @param int $entity_id
+ * @param int $revision_id
+ * @param int $field_name

Little description will help.

+++ b/core/modules/field/field.views.incundefined
@@ -89,18 +113,23 @@ function field_views_field_label($field_name) {
+function field_views_field_default_views_data(FieldInterface $field, $sql_entity_types) {

Doc block need @param and @return.

+++ b/core/modules/field/field.views.incundefined
@@ -447,8 +488,8 @@ function field_views_field_default_views_data($field) {
+function list_field_views_data($field, $sql_entity_types) {

typehint and doc block.

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -305,36 +305,38 @@ public function getInstances($entity_type = NULL) {
+   * @param $entity_type
+   *   The entity type.
    * @param $field_name
...
+  public function getField($entity_type, $field_name) {

Typehint.

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -441,29 +443,31 @@ public function getBundleInstances($entity_type, $bundle) {
+        $loaded_instances = entity_load_multiple('field_instance', array_values($config_ids));

Can we use Drupal::?

I'll try to review remaining patch. Not changing status cuz I don't want anyone to kill me. :D

yched’s picture

FileSize
9.18 KB

Merged & pushed HEAD.

Re @jibran #45: Thanks for the review. Answering below, assuming remarks are numbered.
1. That's for #2056133: Add db_copy_table_schema, that is just included here until it gets in. + I don't think we add @return statements when there is no return value.
2. Nope, 1st line has to be one sentence whenever possible. Then, empty line and additional remarks.
3. I don't think we have an official standard about the order of "use" statements, but why not. And grouping is better indeed. Fixed.
4. Not part of the patch ?
5. 6. 7. Fixed
8. table names are being adjusted in the main issue
9. Not part of the patch
10. Not introduced by the patch + no injection possible in entity controllers for now
11. Same as 2.
12. Fixed
13. This code is just moved around as is, let's keep it that way, there will be issues to remove module_invoke_all()
14. Fixed
15. Why not. Fixed.
16. That comment is moot anyway. Removed.
17. Same as 2. Fixed to split the second sentence to a separate paragraph.
18. Same as 13. This patch is too massive to do that here.
19. 20. 21. Fixed.
22. True, but _update_8000_field_sql_storage_write() is just copied as is from the previous field_sql_storage.install, and I'm still pondering about it. Left as is for now.
23. Fixed
24. Already the case in HEAD, patch is big enough.
25. It's true for most of FieldInfo methods, but fixed this block.
26. Already the case in HEAD, not for this patch.

Pushed the attached interdiff

jibran’s picture

@yched Thanks for explaining all the points in detail.
Review of remaining patch as promised.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -452,7 +423,7 @@ public function delete() {
+          $instance_ids[] = "$entity_type.$bundle.$this->name";

Can we use "$entity_type.$bundle.{$this->name}".

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -7,6 +7,7 @@
+use Drupal\Core\Entity\DatabaseStorageController;
 use Drupal\Core\Language\Language;
 use Drupal\Core\Entity\EntityInterface;

Order :)

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldTestBase.phpundefined
@@ -66,9 +68,10 @@ function getLastFileId() {
+  function createFileField($name, $entity_type, $bundle, $field_settings = array(), $instance_settings = array(), $widget_settings = array()) {

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -98,19 +98,21 @@ function taxonomy_term_feed(Term $term) {
+function taxonomy_autocomplete($entity_type, $field_name) {

+++ b/core/modules/taxonomy/taxonomy.views.incundefined
@@ -335,8 +336,8 @@ function taxonomy_views_data_alter(&$data) {
+function taxonomy_field_views_data($field, $sql_entity_types) {

@@ -363,8 +364,8 @@ function taxonomy_field_views_data($field) {
+function taxonomy_field_views_data_views_data_alter(&$data, $field, $sql_entity_types) {

typehint missing.

yched’s picture

Thanks.
Fixed, except createFileField(). The whole FileFieldTestBase.php is not correctly hinted, including the sister attachFileField() method; and at this point we're really trying to limit the patch size.

yched’s picture

_update_8000_field_sql_storage_write() was still calling field_sql_storage module functions directly, not sure how block upgrade passed so far.
Checking a candidate fix.

yched’s picture

Attempt at removing hook_field_storage_pre_[load|insert|update]()

yched’s picture

And attempt at handling "table name too long" as per #1497374-146: Switch from Field-based storage to Entity-based storage

yched’s picture

Alright, then attempt to revert the test changes that were made to avoid "table name too long"

chx’s picture

Fantastic. fubhy suggested that we re-enable (!) the modules before uninstalling them so to remove the hacking of EntityManager.

I will explore this during the week.

Edit: nope. berdir points out that enabling might be a problem as dependencies might be missing. We will just leave it in and add a todo to kill the hack when disabled modules are gone.

yched’s picture

@chx: OK - you take care of the the todo ?

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-72.patch, failed testing.

yched’s picture

Doh, simpletest pefixes are 16 chars, not 14...
Fixed that, and moved the truncate / hash code to a helper method (see patch in the main issue)

yched’s picture

Status: Needs work » Needs review
FileSize
23.11 KB
483.06 KB

Trying to fix views integration, as mentioned in #1497374-158: Switch from Field-based storage to Entity-based storage

swentel’s picture

Testing refactoring of FieldInfo::getFieldMap() - $field->getBundles() needed refactoring immediately too.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-78.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
484.88 KB

Views intagration - a couple fixes

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-80.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
484.29 KB

Some obvious fixes for infomap/getbundles

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-82.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
11.31 KB
484.4 KB

With another obvious fix

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-84.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
485.69 KB

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-86.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
16.64 KB
486.62 KB

This should be green. @yched I also had to change code in field.views.inc so I'll push once this one comes back green.

yched’s picture

Hah, this one should hopefully be green too :-)
But no pb, I can reroll on top of your changes ($field['bundles'], right ?)

swentel’s picture

$field['bundles'] indeed. Bot hasn't even started yet and many waiting in the queue, looks like the merge will probably be tomorrow :)

yched’s picture

@swentel - remarks on #88:

+++ b/core/modules/field/field.info.incundefined
@@ -74,18 +74,20 @@ function field_behaviors_widget($op, $instance) {
+   *  An array keyed by entity type. Each value is an array which keys are
+   *  field names and value is an array with two entries:
+   *   - type: The field type.

Space added at the wrong place ;-)

+++ b/core/modules/field/field.views.incundefined
@@ -70,10 +70,10 @@ function field_views_data_alter(&$data) {
+  foreach ($field['bundles'] as $bundle) {
     try {
-      if ($entity_manager->getStorageController($entity_type) instanceof DatabaseStorageController) {
-        $sql_entity_types[] = $entity_type;
+      if ($entity_manager->getStorageController($field->entity_type) instanceof DatabaseStorageController) {
+        $sql_entity_types[] = $field->entity_type;

This was looping on the entity_types, so it looks like the foreach is not needed at all now ?

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -184,6 +184,7 @@ public function getFieldMap() {
       $this->fieldMap = $map;
 
       return $map;
+
     }

Needless empty line :-p

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -192,7 +193,7 @@ public function getFieldMap() {
-        $fields[$field_config['uuid']] = $field_config;
+        $fields[$field_config['entity_type']][$field_config['uuid']] = $field_config;
       }

Shouldn't be needed, it can stay keyed by field UUID ?

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -441,9 +442,13 @@ public function getBundleInstances($entity_type, $bundle) {
+      foreach ($this->getFieldMap() as $map_entity_type => $fields) {
+        if ($map_entity_type == $entity_type) {
+          foreach ($fields as $field_name => $field_data) {
+            if (in_array($bundle, $field_data['bundles'])) {
+              $config_ids["$entity_type.$field_name"] = "$entity_type.$bundle.$field_name";
+            }
+          }

Then just iterate on $field_map[$entity_type] ?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -682,13 +680,13 @@ public function hasData() {
-    foreach ($this->getBundles() as $entity_type => $bundle) {
+    foreach ($this->getBundles() as $bundle) {
       // Entity Query throws an exception if there is no base table.
-      $entity_info = \Drupal::entityManager()->getDefinition($entity_type);
+      $entity_info = \Drupal::entityManager()->getDefinition($this->entity_type);
       if (!isset($entity_info['base_table'])) {
         continue;
       }
-      $query = $factory->get($entity_type);
+      $query = $factory->get($this->entity_type);

Similarly, we just use the entity_type, so the loop is not needed anymore ?

+++ b/core/modules/rest/lib/Drupal/rest/LinkManager/RelationLinkManager.phpundefined
@@ -74,14 +74,14 @@ public function getRelations() {
-    foreach (field_info_fields() as $field_info) {
-      foreach ($field_info['bundles'] as $entity_type => $bundles) {
-        foreach ($bundles as $bundle) {
-          $relation_uri = $this->getRelationUri($entity_type, $bundle, $field_info['field_name']);
+    foreach (field_info_fields() as $entity_type => $fields) {
+      foreach ($fields as $field_name => $field_info) {

Whoops. That's correct, but we haven't updated the phpdocs for field_info_fields() & FieldInfo::getFields()
The code to filter out deleted fields in field_info_fields() needs to be updated as well...
My bad :-/, I'll fix that.

(Minor: while we're in there, $field_info could be renamed just $field here.)

Also, keys are not field names here, you need to use $field->name (or $field['field_name'] to keep BC-style)

yched’s picture

Regarding the last snippet - I was confused, it's worse than that actually.
field_info_fields() is currently broken, it still returns an array of fields keyed by field names, but that's wrong, there could be several fields with the same name in different entity types.

It needs to return an array keyed by entity type then field name, and then all current uses need to be updated as well :-/
Let's attack this after the current pending changes have been merged though...

So for now, RelationLinkManager::getRelations() should still assume a flat array of fields keyed by names.

swentel’s picture

Right, sometimes you focus so hard on just getting something green .. :)

Update with interdiff on the interdiff. So yeah, RelationLinkManager::getRelations(), that's a weird one, it failed during refactoring, so somehow this magically works, but I'm too tired now how exactly it can work though.

field_info_fields() is currently broken, it still returns an array of fields keyed by field names, but that's wrong, there could be several fields with the same name in different entity types.

Hmm, well fieldInfo->getFields() stores the field by uuid, so I think we're safe no (in regards of uniqueness at least) ?

Also, in getFieldMap(), there's this little piece of comment, especially the second part. We don't check on unknown entity types - not before either - should we add a check here ? There's a check though in getBundleInstances() - but again, a bit too tired right now to recheck. Minor is that 'type' is always overridden, but I guess that's not to critical.

      // Filter out instances of inactive fields, and instances on unknown
      // entity types.
      if (isset($fields[$field_uuid])) {
        $field = $fields[$field_uuid];
        $map[$instance_config['entity_type']][$field['name']]['bundles'][] = $instance_config['bundle'];
        $map[$instance_config['entity_type']][$field['name']]['type'] = $field['type'];
      }

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-93.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Hmm, well fieldInfo->getFields() stores the field by uuid, so I think we're safe no (in regards of uniqueness at least) ?

Right, fieldInfo->getFields() is good, but field_info_fields() has additional code to filter out deleted fields and re-key by field name. It needs to either re-key by entity_type then field_name, or just keep the UUID keys. Either way, phpdoc & consuming code needs to be updated. Well, let's get this green, and we'll see that later.

re: getFieldMap() / "Filter out instances of inactive fields, and instances on unknown entity types"
Hmm, tricky. I opened #2059965: Should FieldInfo::getFieldMap() filter data about unknown entity types. Let's leave it as is for now :-)

The '*' chars are misaligned in the phpdoc for field_info_field_map(), wrong copy paste from getFieldMap() ;-)

yched’s picture

And right, 'type' gets overwritten over and over again in getFieldMap(), but it's always the same type, so no need to bother with "set it only the first time around".

swentel’s picture

Should be better, somehow missed my views changes in the last patch. No interdiff. Changed the '*' chars (djeezus man) and refactored RelationLinkManager::getRelations().

Also noticed that hook_field_views_data_views_data_alter() is not documented in any api file. I didn't refactor _field_views_get_sql_entity_types() (just return string instead of array) yet as that would trigger a lot of other changes as well. let's spin that off in a follow up ?

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-97.patch, failed testing.

yched’s picture

yes, _field_views_get_sql_entity_types() is removed in the 'views intergration refactor patch'

swentel’s picture

Status: Needs work » Needs review
FileSize
486.88 KB

Right, and that's somehow being annoying at this point. Going for another botcheck first, but some views tests are failing.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-99.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
30.17 KB
492.95 KB

Ok, I've merged your patch with mine, let's see - one local test was already ok, so good hopes. interdiff attached.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-102.patch, failed testing.

swentel’s picture

right, forgot to update the hook implementations ...

swentel’s picture

Status: Needs work » Needs review
FileSize
15.42 KB
510.97 KB

new patch after yched pushed his views refactoring.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-105.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
15.88 KB
496.33 KB

man ...

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-107.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
496.15 KB

That was a nasty one. I'll push when it comes back green.

yched’s picture

Do not use truncated table names in views_data entries.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-110.patch, failed testing.

chx’s picture

yched what else is left, i saw the green in #172 in the parent

yched’s picture

Status: Needs work » Needs review
FileSize
573.34 KB

Testing reroll this time...

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-113.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
574.25 KB

Missed one change.

yched’s picture

Reroll

yched’s picture

as per the end of #1497374-212: Switch from Field-based storage to Entity-based storage:
- call fieldOp() methods explicitly instead of within invokeHook()
- merge fieldUpdate() & fieldInsert() into fieldSave()

yched’s picture

Reroll

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-118.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
604.36 KB

Method renames

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-120.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
0 bytes
chx’s picture

FileSize
510.95 KB
yched’s picture

Method renames (same as #120) + minor adjustements + chx's reroll.

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-124.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
598.55 KB

Re-reroll...

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-126.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
603.91 KB

forum.module changes got reverted in a previous reroll...

Status: Needs review » Needs work

The last submitted patch, field_storage-testbot-2047777-127.patch, failed testing.

yched’s picture

Sigh... Pushed yet another reroll, but those fails beat me for today...

yched’s picture

Status: Needs work » Needs review
FileSize
603.97 KB

OK, I think I got it.

chx’s picture

FileSize
126.06 KB

yched, congrats on getting it to green again but I think you deleted field_sql_storage.module which caused the patch to swell to 600kb. But I can't get it back down to 500kb now it seems. I have made an interdiff.

yched’s picture

@chx: the patch I post in the helper issue is the "FULL" patch (with old hook docs & field_sql_storage.module deleted and its tests re-added).
The sandbox contains the "REVIEW" patch - except I forgot to push the fix from #131 yesterday night :-/. Done now.

Will post an updated patch in the main issue with a summary of the changes & interdiffs.

jibran’s picture

Status: Needs review » Fixed

Yay!!!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.