Easier DX : get the $field from the $instance object.
Thus, rename the FieldInstance::$field_name property.

Files: 
CommentFileSizeAuthor
#21 field-instance-getField-1967106-21.patch13.01 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 54,726 pass(es).
[ View ]
#21 interdiff.txt748 bytesswentel
#15 field-instance-getField-1967106-15.patch13.01 KByched
PASSED: [[SimpleTest]]: [MySQL] 54,472 pass(es).
[ View ]
#15 interdiff.txt11.73 KByched
#10 get-field-1967106.10.interdiff.txt455 byteslarowlan
#10 get-field-1967106.10.patch6.25 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,450 pass(es).
[ View ]
#8 get-field-1967106.8.patch6.22 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Tagging

Issue tags:+Field API

tagging

Status:Active» Postponed

Postponing on the main issue.

Priority:Normal» Major

This should probably be one of the 1st followup we tackle, since it will have major impact on the shape of the code rewrites that happen in #1953408: Remove ArrayAccess BC layer from field config entities.

Un-postponing.

Status:Postponed» Active

Assigned:Unassigned» larowlan

Sounds straight forward

Status:Active» Needs review
StatusFileSize
new6.22 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

like so?

Status:Needs review» Needs work

The last submitted patch, get-field-1967106.8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.25 KB
PASSED: [[SimpleTest]]: [MySQL] 54,450 pass(es).
[ View ]
new455 bytes

Missed the namespace

+++ b/core/modules/field/lib/Drupal/field/FieldInstanceInterface.phpundefined
@@ -0,0 +1,19 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\field\FieldInstanceInterface.
+ */
+
+namespace Drupal\field;
+
+interface FieldInstanceInterface {
+
+  /**
+   * Returns the field entity for this field instance.
+   *
+   * @return \Drupal\field\Plugin\Core\Entity\Field.php
+   *   The field entity for this instance.
+   */
+  public function getField();
+}

Why are we adding an interface? Is this the only method on FieldInstance that doesn't belong to ConfigEntityBase or ArrayAccess? Interesting.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -51,6 +52,13 @@ class FieldInstance extends ConfigEntityBase implements \ArrayAccess {
   /**
+   * The field to which this instance belongs.
+   *
+   * @var \Drupal\field\Plugin\Core\Entity\Field
+   */
+

Seems to be missing the actual declaration? :)

Yay, thanks @larowlan for jumping on this :-)

Yeah, I'm not sure about the interface either (and yes, there are other methods in FieldInstance currently that would need to go to the interface if we introduced one).

Sure, we will definitely be hinting on FieldInstance in function / method params (we already currently are at places, and the number will grow in #1953408: Remove ArrayAccess BC layer from field config entities), and core standards state that we should favor hinting on interfaces.
OTOH, this is not really related to this very task, and
- no other existing ConfigEntity in core right now provide their own interface, aside from implementing ConfigEntityInterface itself.
- the situation is a bit blurry for content entities, and is being discussed in #1391694: Use type-hinting for entity-parameters.

So I'd really suggest leaving this question of interfaces aside for now.

Status:Needs review» Needs work

// @todo Revisit that in favor of a getField() method.

So, If we consider that we are in the task that is supposed to get rid of that @todo ;-), we should:

  • remove the $field_name property
  • in __construct(), 1st code branch:

        // Accept incoming 'field_name' instead of 'field_uuid', for easier DX on
        // creation of new instances.
        if (isset($values['field_name']) && !isset($values['field_uuid'])) {
          $this->field = field_info_field($values['field_name']);
          if ($this->field) {
            $values['field_uuid'] = $this->field->uuid;
         }

    Good, but then also unset($values['field_name'])
  • in __construct(), 2nd code branch:

        elseif (isset($values['field_uuid']) && !isset($values['field_name'])) {
          $this->field = current(field_read_fields(array('uuid' => $values['field_uuid']), array('include_inactive' => TRUE, 'include_deleted' => TRUE)));
          if ($this->field) {
            $values['field_name'] = $this->field->id;
          }
          else {
            throw new FieldException(format_string('Attempt to create an instance of unknown field @uuid', array('@uuid' => $values['field_uuid'])));
          }

    No more assigning $values['field_name'], just raise the exception if (empty($this->field))
  • in offsetGet() :
    add a case to return $this->field->id when $offset is 'field_name', to keep the BC $instance['field_name'] syntax working.
  • - Additionally, in save():

          if (empty($this->field)) {
            throw new FieldException(format_string("Attempt to save an instance of a field @field_id that doesn't exist or is currently inactive.", array('@field_name' => $this->field_name)));
          }

    It looks like the code in __construct() has wiped any chance of this actually happening ?

Assigned:larowlan» yched

This is going to be useful in several places. Taking a crack at this.

Status:Needs work» Needs review
StatusFileSize
new11.73 KB
new13.01 KB
PASSED: [[SimpleTest]]: [MySQL] 54,472 pass(es).
[ View ]

Patch does as described in #13.

Additionally:
- Takes care of FieldInstance serialization/unserialization : we don't want to serialize $this->field within the $instance.
Also needed because $instance also persists the widget Plugin object ($this->widgetPlugin), and we don't want that one serialized either (FieldInstance had a __sleep() method that took care of that, but it got lost in #1735118: Convert Field API to CMI)
- While I'm at it, does the same for Field, since that stores derived data that we don't want serialized either (field schema, storage details).

So the interface is gone? I think that as a general rule any methods on an entity not from {Content|Config}EntityInterface should warrant a new interface.

@larowlan : see #11 & #12.

I think that as a general rule any methods on an entity not from {Content|Config}EntityInterface should warrant a new interface.

Possibly, but FieldInstance already has methods that are not part of ConfigEntityInterface, the patch doesn't change that fact. So let's not tie it to this discussion when it doesn't need to :-)

Fair point, interface is scope creep for this issue

That will be true after #1391694: Use type-hinting for entity-parameters, I updated that to have interfaces for both Field and FieldInstance

Assigned:yched» Unassigned

Unassigning. Needs reviews now :-)

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new748 bytes
new13.01 KB
PASSED: [[SimpleTest]]: [MySQL] 54,726 pass(es).
[ View ]

Solid cleanup, me like. Nitpick patch attached (redundant space). Assuming it's still green :)

Priority:Major» Normal
Status:Reviewed & tested by the community» Fixed

This looks like a nice DX improvement, but in no way is it major. :)

Committed and pushed to 8.x. Thanks!

FYI : #1977206: Default serialization of ConfigEntities - proposal to move the serialize() / unserialize() methods from Field / FieldInstance to ConfigEntityBase.

@webchick: yeah, sorry about the "major" thing, I guess got a bit carried away.
Was mostly a way to help us sort out priorities between the numerous followup issues opened after the Field / CMI patch, but right, bad idea...

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