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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Tagging

swentel’s picture

Issue tags: +Field API

tagging

xjm’s picture

Status: Active » Postponed

Postponing on the main issue.

yched’s picture

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.

yched’s picture

Un-postponing.

yched’s picture

Status: Postponed » Active
larowlan’s picture

Assigned: Unassigned » larowlan

Sounds straight forward

larowlan’s picture

Status: Active » Needs review
FileSize
6.22 KB

like so?

Status: Needs review » Needs work

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
6.25 KB
455 bytes

Missed the namespace

xjm’s picture

+++ 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? :)

yched’s picture

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.

yched’s picture

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 ?

yched’s picture

Assigned: larowlan » yched

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

yched’s picture

Status: Needs work » Needs review
FileSize
11.73 KB
13.01 KB

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

larowlan’s picture

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.

yched’s picture

@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 :-)

larowlan’s picture

Fair point, interface is scope creep for this issue

tim.plunkett’s picture

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

yched’s picture

Assigned: yched » Unassigned

Unassigning. Needs reviews now :-)

swentel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
748 bytes
13.01 KB

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

webchick’s picture

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!

yched’s picture

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.