Files: 
CommentFileSizeAuthor
#30 1981306-30.patch49.06 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 57,657 pass(es).
[ View ]
#27 1981306-27-sandbox-do-not-test.patch12.53 KBpcambra
#26 1981306-26.patch49.07 KBpcambra
PASSED: [[SimpleTest]]: [MySQL] 58,293 pass(es).
[ View ]
#24 interdiff.txt39.09 KBandypost
#24 1981306-24.patch47.39 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,019 pass(es), 12 fail(s), and 4 exception(s).
[ View ]
#23 1981306-23.patch45.36 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 57,203 pass(es).
[ View ]
#21 1981306-20.patch45.31 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 57,551 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#18 1981306-18.patch28.67 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 55,779 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 1981306-16.patch27.82 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 56,463 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#14 1981306-14.patch27.5 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 55,521 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 1981306-9.patch22.37 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,329 pass(es).
[ View ]
#1 1953410-cud-1981306-1.patch21.63 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1953410-cud-1981306-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new21.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1953410-cud-1981306-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Issue tags:+Field API

taggin

Issue tags:+Novice

tagged to office hours

Status:Needs review» Closed (duplicate)
Issue tags:-Novice

Status:Closed (duplicate)» Closed (won't fix)

We're going todo this in one patch.

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

So we agreed to this in chunks anyway.

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.

Status:Needs work» Needs review
StatusFileSize
new22.37 KB
PASSED: [[SimpleTest]]: [MySQL] 55,329 pass(es).
[ View ]

rerolled

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 ?

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.

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 .. :/

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.

StatusFileSize
new27.5 KB
FAILED: [[SimpleTest]]: [MySQL] 55,521 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Let's try this one

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new27.82 KB
FAILED: [[SimpleTest]]: [MySQL] 56,463 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Less fatals hopefully

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new28.67 KB
FAILED: [[SimpleTest]]: [MySQL] 55,779 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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..

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.

Status:Needs work» Needs review
StatusFileSize
new45.31 KB
FAILED: [[SimpleTest]]: [MySQL] 57,551 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Another try... Refactored some more :s

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new45.36 KB
PASSED: [[SimpleTest]]: [MySQL] 57,203 pass(es).
[ View ]

Hmm

StatusFileSize
new47.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,019 pass(es), 12 fail(s), and 4 exception(s).
[ View ]
new39.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.

Status:Needs work» Needs review
StatusFileSize
new49.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,293 pass(es).
[ View ]

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.

StatusFileSize
new12.53 KB

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

Status:Needs review» Reviewed & tested by the community

Fantastic cleanup andypost and pcambra, thanks!

@pcambra Merged to sandbox, thanx a lot

StatusFileSize
new49.06 KB
PASSED: [[SimpleTest]]: [MySQL] 57,657 pass(es).
[ View ]

Status:Reviewed & tested by the community» Fixed

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

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.