Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

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

Issue tags: +Field API

taggin

andypost’s picture

Issue tags: +Novice

tagged to office hours

aspilicious’s picture

Status: Needs review » Closed (duplicate)
Issue tags: -Novice
swentel’s picture

Status: Closed (duplicate) » 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: -Field API

#1: 1953410-cud-1981306-1.patch queued for re-testing.

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

The last submitted patch, 1953410-cud-1981306-1.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
22.37 KB

rerolled

swentel’s picture

Assigned: Unassigned » yched

This looks pretty neat to me, nearly RTBC, apart from one that I'm not sure of. Assigning to yched for opinion.

+++ b/core/modules/comment/comment.installundefined
@@ -10,7 +10,7 @@
+  field_info_field('comment_body')->delete();

Should we do entity_load('id')->delete() here ?

yched’s picture

re #10: Unless I'm missing something, I don't think it makes an actual difference, in the end it's the same object that gets deleted.
field_info_field() is still the performance-friendly way to access a Field entity, so it's probably still the one we should promote vs. entity_load(), although both will work here.

Only nitpicks below, I'll let you judge whether you think this is worth fixing.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockFieldTest.phpundefined
@@ -64,21 +64,21 @@ public function testBlockFields() {
       'field_name' => $this->field['field_name'],
       'entity_type' => 'custom_block',
       'bundle' => 'link',
       'settings' => array(
         'title' => DRUPAL_OPTIONAL,
       ),
-    );
-    field_create_instance($this->instance);
+    ));
+    $this->instance->save();
     entity_get_form_display('custom_block', 'link', 'default')
       ->setComponent($this->field['field_name'], array(

(+ other places down in the method)
$this->field is now a Field entity rather than an array, so technically this still works thank to the ArrayAccess BC, and we have a separate issue to get rid of the BC, but strictly speaking this is adding a *new* use of the BC, which we might want to avoid ?
(meaning, depending on the order of patches, it might slip through the cracks and not get removed)
Your call :-)

+++ b/core/modules/contact/lib/Drupal/contact/Tests/Views/ContactFieldsTest.phpundefined
@@ -39,19 +39,17 @@ public static function getInfo() {
+    entity_create('field_instance', array(
+      'field_name' => $this->field['field_name'],
       'entity_type' => 'contact_message',

Same here, and in the testViewsData() method lower.

+++ b/core/modules/email/lib/Drupal/email/Tests/EmailFieldTest.phpundefined
@@ -50,13 +50,13 @@ function testEmailField() {
+    entity_create('field_entity', $this->field)->save();
     $this->instance = array(
       'field_name' => $this->field['field_name'],

Same

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.phpundefined
@@ -144,13 +144,13 @@ public function testFieldComponent() {
-    field_create_field($field);
+    entity_create('field_entity', $field)->save();

No biggie, might be clearer to ditch the array variables and have the rest of the test work on the Entities, with object syntax ?
(same in EntityFormDisplayTest)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceAutoCreateTest.phpundefined
@@ -46,7 +46,7 @@ function setUp() {
-    field_create_field($field);
+    entity_create('field_entity', $field)->save();

The $field and $instance arrays are not needed, could be inlined.

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceSelectionSortTest.phpundefined
@@ -41,7 +41,7 @@ public function testSort() {
-    field_create_field($field_info);
+    entity_create('field_entity', $field_info)->save();

Same for the $field_info / $instance_info arrays here.

swentel’s picture

Assigned: yched » Unassigned

meaning, depending on the order of patches, it might slip through the cracks and not get removed

Huh ? I think whoever comes first, that will be removed anyway no ? Or am I missing something completely ? Of course, it might probably be easier todo that here, because it's going to clash like hell of course .. :/

yched’s picture

Yeah, dunno what would exactly happen - since those are places that currently don't rely on the ArrayAccess BC, chances are they might not be affected by the other patch, so there would be no conflict... Might be tedious or not.
But yeah, point is, we might want to avoid introducing new uses of the BC layer in places that currently don't use it.

aspilicious’s picture

FileSize
27.5 KB

Let's try this one

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.82 KB

Less fatals hopefully

Status: Needs review » Needs work

The last submitted patch, 1981306-16.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
28.67 KB

Needed a reroll anyway... Lets see if this could work...

I have the feeling

$this->field = entity_create('field_entity', array(
      'field_name' => drupal_strtolower($this->randomName()),
      'type' => 'email',
    ))->save();

isn't supported.

Tests fail when you assign the creation to a variable and save it at the same time..

yched’s picture

Yup. Tedious in practice, but save() returns a flag indicating "created or updated".
So $entity = entity_create(...)->save(); doesn't work.

Status: Needs review » Needs work

The last submitted patch, 1981306-18.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
45.31 KB

Another try... Refactored some more :s

Status: Needs review » Needs work

The last submitted patch, 1981306-20.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
45.36 KB

Hmm

andypost’s picture

FileSize
47.39 KB
39.09 KB

A bit more clean-up, most of places with field->id() changed to $field_name variable to make it easy to refactor latter
Also pushed to sandbox

PS: @aspilicious do you have access to sandbox?

Status: Needs review » Needs work

The last submitted patch, 1981306-24.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
49.07 KB

Rerroll of #24.

The only changes are due to the $field name
"$field_name[$langcode][0][value]" -> "{$field_name}[$langcode][0][value]"

And better type hinting:

  /**
   * A field to use in this test class.
   *
   * @var \Drupal\field\Plugin\Core\Entity\Field -> instead of array
   */
  protected $field;

  /**
   * The instance used in this test class.
   *
   * @var \Drupal\field\Plugin\Core\Entity\FieldInstance -> instead of array
   */
  protected $instance;

Sorry for the lack of interdiff, hopefully this is green.

pcambra’s picture

Actually just realized that the interdiff should be applied to the sandbox so here it is for #26

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Fantastic cleanup andypost and pcambra, thanks!

andypost’s picture

@pcambra Merged to sandbox, thanx a lot

amateescu’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2de4615 and pushed to 8.x. Thanks!

fubhy’s picture

Can someone check whether all the places where we replaced field_delete_field() with $field->delete() are actually safe to do so? field_delete_field() has a if() check for whether the field actually exists. The places where we replaced it with the new syntax don't have that check. @see #2022769: Fix conversion of field_delete_field() in hook_uninstall()

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