Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Postponed
swentel’s picture

Issue tags: +Field API

tagging

yched’s picture

Status: Postponed » Active

Un-postponing.

swentel’s picture

Actually, we test it for the body instance:

    foreach (array('article', 'page') as $node_type) {
      $config = config("field.instance.node.$node_type.body")->get();

So I think we're good imo.

yched’s picture

That's just for the upgrade path.
I think @xjm was thinking of the config import tests.

swentel’s picture

right, duh, I need to learn to read titles.

swentel’s picture

Status: Active » Needs review
FileSize
5.13 KB

Status: Needs review » Needs work

The last submitted patch, 1960574-field-cmi-more-config-tests.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
9.28 KB

Right, more fields, means more stuff to delete :)

smiletrl’s picture

Nice job!
Some nitpicks though:^

FieldImportCreateTest.php:

+    // A field with multiple instances.
+    $field_2 = entity_load('field_entity', $field_id);
+    $this->assertTrue($field, 'The second field was created.');

I think here should be

+    // A field with multiple instances.
+    $field_2 = entity_load('field_entity', $field_id_2);
+    $this->assertTrue($field_2, 'The second field was created.');
     // Check that the field and instance do not exist yet.
     $this->assertFalse(entity_load('field_entity', $field_id));
     $this->assertFalse(entity_load('field_instance', $instance_id));

This patch doesn't check $field_2 and $instance_2 before enabling field_test_config module.

It might be a good thing to add this multiple instances test in

  /**
   * Tests creating fields and instances during config import.
   */
  function testImportCreate() {

// Add the coresponding entries to the current manifest data.
This is not from the patch, but in the original code -- wrong word *corresponding*.

FieldImportDeleteTest.php:

+    $instance_id_2a = "test_entity.test_bundle.$field_id";
+    $instance_id_2b = "test_entity.test_bundle.$field_id_2";

I think you want $instance_id_2b to has a different bundle -- + $instance_id_2b = "test_entity.test_bundle_2.$field_id_2"; and $instance_id_2a has a different field id -- $field_id_2:)

-    // Check that the config was correctly imported.
-    $field = entity_load('field_entity', $field_id);
-    $this->assertTrue($field, 'The field was created.');
-    $instance = entity_load('field_instance', $instance_id);
-    $this->assertTrue($instance, 'The field instance was created.');

Any particular reason to delete these checks? In this test file, it uses $this->installConfig(array('field_test_config')); to enable configuration, different from

module_enable(array('field_test_config'));

in FieldImportCreateTest.php. I think it's necessary to add this check and for new field and instances in this patch.

-    $field_uuid = $field->uuid;
+    // Get the uuid's for the fields.
+    $field_uuid = entity_load('field_entity', $field_id)->uuid();
+    $field_uuid_2 = entity_load('field_entity', $field_id)->uuid();

Could be $field_uuid_2 = entity_load('field_entity', $field_id_2)->uuid();, although I prefer this way $field->uuid^^

Finally, while this patch creates a new field and two based different instances, I guess a second instance with a different bundle based on current field in these tests will be okay:)

smiletrl’s picture

Patch updated according to #10.

For 'uuid' in staging yml file, I used following code to generate them. Not sure I did it correctly.

use Drupal\Component\Uuid\Uuid;
$ob = new Uuid();
$uuid = $ob->generate();
dsm($uuid);
$uuid = $ob->generate();
dsm($uuid);
$uuid = $ob->generate();
dsm($uuid);

Status: Needs review » Needs work

The last submitted patch, more_tests_for_field_config-1960574-10.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
8.79 KB
16.86 KB

Fixed some bugs.

swentel’s picture

Status: Needs review » Needs work

Needs a reroll

error: patch failed: core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.php:68
error: core/modules/field/lib/Drupal/field/Tests/FieldImportDeleteTest.php: patch does not apply
YesCT’s picture

Issue tags: +Needs reroll

tagging.
also, instructions if someone new wants to try it: http://drupal.org/patch/reroll

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.88 KB
swentel’s picture

Status: Needs review » Reviewed & tested by the community

This is good to go btw, mine was just a re-roll.

swentel’s picture

Status: Reviewed & tested by the community » Needs work
swentel’s picture

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

And here's the reroll.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 939a8a8 and pushed to 8.x. Thanks!

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