Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
15.16 KB
andypost’s picture

Issue tags: +Novice

tagged to office hours

swentel’s picture

Status: Needs review » Closed (won't fix)

We're going todo this in one patch.

swentel’s picture

Status: Closed (won't fix) » Needs review

So we agreed to this in chunks anyway.

swentel’s picture

Issue tags: -Novice, -Field API

#1: 1981292-1.patch queued for re-testing.

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

The last submitted patch, 1981292-1.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
10.35 KB

Re-rolled - it's a bit smaller now that entityformdisplay is in.

Status: Needs review » Needs work

The last submitted patch, 1981292-7.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
13.34 KB

Lets see how hard this fails (or not)

Status: Needs review » Needs work

The last submitted patch, 1981292-9.patch, failed testing.

aspilicious’s picture

Not everything seems related, lets retest

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Field API

#9: 1981292-9.patch queued for re-testing.

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

The last submitted patch, 1981292-9.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

Should be better

aspilicious’s picture

FileSize
16.42 KB

Status: Needs review » Needs work

The last submitted patch, 1981292-14.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
17.5 KB

Hmm

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Tests/FieldSqlStorageTest.phpundefined
@@ -261,7 +265,7 @@ function testFieldAttachSaveMissingData() {
-      ->values(array($entity_type, $this->instance['bundle'], 0, 0, 0, 0, $unavailable_langcode))
+      ->values(array($entity_type, $this->instance->bundle], 0, 0, 0, 0, $unavailable_langcode))

bundle)

pcambra’s picture

Status: Needs work » Needs review
FileSize
17.42 KB

Patch had a couple of syntax errors on FieldSqlStorageTest so I fixed those and removed a non-desired Views dependency changing a method.
Also improved the documentation and fixed some schema getting.

The patch won't pass yet, though. I think we've found an inconsistency here on the foreign key naming/expected naming, on testFieldSqlStorageForeignKeys test method, at the bottom, we have the following:

    $this->assertEqual(count($schema['foreign keys']), 1, 'There is 1 foreign key in the schema');
    $foreign_key = reset($schema['foreign keys']);
    $foreign_key_column = _field_sql_storage_columnname($field['field_name'], $foreign_key_name);
    $this->assertEqual($foreign_key['table'], $foreign_key_name, 'Foreign key table name preserved in the schema');
    $this->assertEqual($foreign_key['columns'][$foreign_key_column], 'id', 'Foreign key column name preserved in the schema');

So, $foreign_key['columns'][$foreign_key_column] is expecting "id" whereas the field storage column name is generating a very different column name (testfield_color):

function _field_sql_storage_columnname($name, $column) {
  return in_array($column, Field::getReservedColumns()) ? $column : $name . '_' . $column;
}

Shall I replace

$this->assertEqual($foreign_key['columns'][$foreign_key_column], 'id', 'Foreign key column name preserved in the schema');

By

$this->assertEqual($foreign_key['columns'][$foreign_key_column], 'testfield_color', 'Foreign key column name preserved in the schema');

?

Status: Needs review » Needs work

The last submitted patch, 1981292-19.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
17.27 KB
1.01 KB

The test failing is actually a sql schema test, using _field_sql_storage_schema should fix it.

aspilicious’s picture

Shouldn't we use $field->getSchema() (or something like that). I saw that function in the field object.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Tests/FieldSqlStorageTest.phpundefined
@@ -506,6 +506,8 @@ function testFieldSqlStorageForeignKeys() {
+    $schemas = _field_sql_storage_schema($field);
+    $schema = $schemas[_field_sql_storage_tablename($field)];

This functions is not deprecated so ok

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0eeda29 and pushed to 8.x. Thanks!

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