Problem/Motivation

This is a followup from #1735118: Convert Field API to CMI and postponed on that issue.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+  /**
+   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct().
+   */

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -0,0 +1,423 @@
+  /**
+   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct().
+   */
+  public function __construct(array $values, $entity_type = 'field_instance') {

I feel like we should document what we expect to have in the $values array here.
E.g., the exception handling for Field implies that the field type and field_name are required, that the the field_name needs to be a valid machine name.
For FieldInstance, they imply that entity_type, bundle, and one of field_name and field_uuid are required.

Per @swentel:

field_create_*(), field_update_*() document those.

Proposed resolution

Use the documentation from the field CRUD functions as a basis for docs describing the expected inputs for these constructors. An @see reference does not make sense because #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API will remove these functions.

Remaining tasks

  • Review the existing documentation for field_create_field(), field_create_instance(), field_update_field(), and field_update_instance().
  • Create a patch that adds similar documentation to the two class constructors' docblocks.
  • Have field and documentation maintainers review the proposed docs for accuracy.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: +Novice, +Field API
yched’s picture

Status: Postponed » Active

Un-postponing.

smiletrl’s picture

This is a trial for Field Class constructor. I'm not sure why the indentation is different:(

The basic idea is that those properties already have had details description in respective part, e.g.,

  /**
   * The field ID (machine name).
   *
   * This is the name of the property under which the field values are placed in
   * an entity : $entity-{>$field_id}. The maximum length is
   * Field:ID_MAX_LENGTH.
   *
   * Example: body, field_main_image.
   *
   * @var string
   */
  public $id;

So, I guess it will be redundant to type them again. Only identify whether this property is required or not in constructor doc atm.

smiletrl’s picture

Status: Active » Needs review
swentel’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -210,7 +210,29 @@ class Field extends ConfigEntityBase implements FieldInterface {
+	 *
+	 * @param array $values

Good start, but a lot of tabs in the patch.

smiletrl’s picture

This patch contaings both field and field instance. I guess, some properties should not be provided via constructor variables. Like class FieldInstance:

  /**
   * The field ConfigEntity object corresponding to $field_uuid.
   *
   * @var \Drupal\field\Plugin\Core\Entity\Field
   */
  protected $field;

Anyway, include them in this doc atm.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! There are a few formatting/grammar things that need to be fixed in this patch:

a) The patch is not following our generic first-line-in-docblock standards (needs a blank line after the first-line docs) (and this is a couple of times in the patch):

  /**
-   * {@inheritdoc}
+   * Constructs a Field object.
+   * In most cases, it's constructed via entity_create('field_entity', $values).

b) Both of the constructors are missing @param $entity_type documentation.

c) This English wording needs a fix:

+   *   An array of values to set, keyed by property name. For specific property
+   *   description, please refer respective property inside this class.

Should be "For specific property descriptions, please refer to the property documentation inside this class."

d) This English wording needs a fix:

+   *   - cardinality: optional. Default to be 1.

Should be "Defaults to 1.".

e)

+   * See: @link entity_create @endlink.
+   * See: @link field Field API data structures @endlink.

Use @ingroup instead of "See: @link" to link to topics.

f) In lists, we normally want the description after : to start with a capital letter. See
http://drupal.org/node/1354#lists

g) I have not reviewed the accuracy of the documentation; someone else can please review that.

yched’s picture

+ * - id: required. It's used to replace the original 'field_name' property.
'field_name' is only present as a temporary BC layer to keep existing code running, it will be removed altogether from D8 codebase in #1953408: Remove ArrayAccess BC layer from field config entities, at which point it will be only a D7 thing. No need to document the past here, let's not mention 'field_name'.

More generally, all those properties are already documented as class members, I'd strongly object documenting their meaning, default values or even listing them twice, this is bound to get out of sync at some point.
The need mentioned in the OP was about documenting which properties were required in the $values array ?

smiletrl’s picture

Thanks for the review!

For

In lists, we normally want the description after : to start with a capital letter.

This is done in this patch. But I find this rule doesn't apply for most docs in code right now:)

This patch has reduced many properties in doc. The reason I include them all in the doc is because I'm not sure which one will be optional in array $values. I could only confirm that some properties will be required according to construction function.

For a quick review through save funtion inside both classes, I deleted some properties, because obviously, they are going to get set in the save process automatically.

An ambiguous property is $settings. This $settings is optional in field_create_field and field_create_instance functions according to current doc. But in my check,

      // Make sure all settings are present, so that a complete field
      // definition is passed to the various hooks and written to config.
      $this->settings += $field_type['settings'];

$settings will be overridden by elements in $field_type['settings']; So, no sense to provide default values via construct function.

Also, some properties in this patch are ambiguous to me. Need accuracy review:)

smiletrl’s picture

Status: Needs work » Needs review
smiletrl’s picture

Just for the records, at the top parent Class Entity::__construct function, every member variable gets a chance to be initialized by $values array.

public function __construct(array $values, $entity_type) {
  $this->entityType = $entity_type;
  // Set initial values.
  foreach ($values as $key => $value) {
    $this->$key = $value;
  }
}

So, theoretically, every member inside this class could have an initial value via param $values passed to _construct function. That's why I wanted to list them all in _construct doc firstly.

yched’s picture

+ * - Type: required.

Should be 'type'. The actual property names are lowercased, we can't uppercase them in docs.

theoretically, every member inside this class could have an initial value via param $values passed to _construct function. That's why I wanted to list them all in _construct doc firstly.

Sure, that's how the Entity system works for all entity types throughout core. But across all entity types in core right now, be it "content" entities or "config" entities, *no* entity type provide specific documentation on values accepted in __construct(), other that the documentation provided for each class property.

So, definite -1 on listing all properties again in the phpdoc for __construct() *just* on Field / FieldInstance - at least not without discussing this at the level of the entity system as a whole.

What would make sense for the specific case of Field / FieldInstance entities at hand here is just to document which properties are required when creating a new entity.
For Field:
'type'
'id' (as a temporary BC layer right now, a 'field_name' property can be accepted in place of 'id')
For FieldInstance:
either 'field_name' or 'field_uuid'
'bundle'
'entity_type'

smiletrl’s picture

Thanks for clearing that lowercase up!

Well, I don't favor listing them twice either. And yes, it doesn't make much sense to identify which properties are required ONLY in field/field instance's class constructor in the first place.

I think purpose of this issue is trying to create doc for new way of creating field and field instance, i.e., entity_create() instead of field_create_field() . While field_create_field() has sufficient doc for developer to know what variables should/could be passed into to create a field, entity_create() doesn't.

So, when we tell a developer that entity_create('field_entity', $values)->save() would be a replacement of field_create_field() to create a field in D8, in doc we don't tell him/her what is required/expected in $values. I guess that's the problem we are facing.

--------------------------------------------------------------------------------------------------------------------
It's not easy to say where would be the "right place" to put the doc. My first option would be doc for function entity_create() , because that's where developer starts to create field/field instance. We give details of possibly the most widely used examples of entity_create() in contrib code, e.g., field, field instance, block.

The second option would be the construct doc here, because essentially, this is the final place to construct a field object, i.e., $field_object = new Field($values, 'field_entity');.

What would make sense for the specific case of Field / FieldInstance entities at hand here is just to document which properties are required when creating a new entity.

From my eyes, it's beacuse entity field and entity 'field instance' could be created in contrib code frequently. There would be strong expectation for these info, and class construct function would be a good place to give explanations of how to create them. As for other entities, like node or taxonomy term, I guess they are not created in contrib code as widely as field/field instance is, so, no need to give details in construct doc for node. Now, it seems entity block has the similar situation with entity field, since entity block would be created a lot in contrib code, but original block hooks are deprecated with their doc in D8^^

In a world, we need doc to tell how to create field in contrib code, like field_create_field() doc does. And to me, field construct doc could be a good option too:)

jhodgdon’s picture

Status: Needs review » Needs work

The problem here with just saying something like "The keys of $values are the same as names of properties of this class" is that, for instance, FieldInstance extends ConfigEntityBase (which has 2 properties that are inherited), which in turn extends Entity (which has 5 properties that are inherited). So you might have to go searching through two more classes in order to figure out what those keys would be... I guess that is OK, but it should be mentioned in the documentation. Since (as pointed out above) $values is flexibly used to initialize class properties, I agree that we can't really document every possible key.

So let's make sure to say:
- Which elements of $values are definitely required
- That elements of $values [with some exceptions, which I can see at least from the FieldInstance code] are set as properties on the class instance
- That you should look at the documentation of class properties on this class and ones it extends for more information on what the class properties mean

smiletrl’s picture

Discussed with @jhodgdon in irc.

The basic idea is to make it necessary, clear and clean, yet still against @yched's opinion:( Yeah, field / field instance constructors are *really* special cases considering the whole Entity system^^

smiletrl’s picture

Status: Needs work » Needs review
smiletrl’s picture

Wrong word *detials* at #15.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch!

Several of my comments from the review in #7 have not been addressed yet.

Also:

a) Do not use the abbreviation "BC" in documentation. Write it out. Not everyone knows what this is.

b) The documentation for $values is not very clear to me. I think it would be better if it said something like "An array of field properties, keyed by property name. Most array elements will be used to set the corresponding properties on the class; see the class property documentation for details. Some array elements have special meanings and a few are required; these special elements are:"

c) There should always be a : before a list.

d) When I looked at the code for the constructors a few days ago, it seemed to me that there were a couple of array elements that were *not* properties of the class. These should be listed in the documentation (as I implied in (b)).

smiletrl’s picture

Thanks for review!

New patch.

When I looked at the code for the constructors a few days ago, it seemed to me that there were a couple of array elements that were *not* properties of the class. These should be listed in the documentation (as I implied in (b)).

As far as I see, only 'field_name' is not listed in class FieldInstane, and it's mentioned in parameter.

For @param with entity_create(). There's already doc for entity_create(). I guess no need to document it again. But it's a good thing to mention connection between them.

There's heavy work at Make formatters and widgets work on nonconfigurable fields, once it's in, these docs should be updated again.

smiletrl’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch!

I have not checked the details of this patch for accuracy -- can someone else do that?

A few things I did notice that I think can be improved:

a) Both constructors say

+   * In most cases, it's constructed via entity_create('field_entity', $values)), where
+   * $values is the same parameter for this constructor.

This cannot possibly be correct for both of them.

b) The Field constructor refers to "Field", and that makes sense, as it's the class name. The second refers to "Field instance". I think it should say "FieldInstance", since that is its class name, or else both should use "field" in lower-case.

c) The docs for FieldInstance say that $values contains "field properties"... presumably these are field instance properties instead?

d) The wording in the list items in FieldInstance needs work. For instance, field_name should probably say something like "The name of the field this is an instance of." instead of "This field instance is attached to this field". "attached" is a special term in the Field API, and it is misused here, and the double "this" is confusing. The second list item's wording coudl be improved too.

e) We do have a standard way of indicating optional/required in lists - http://drupal.org/node/1354#lists

f) BC means Backwards Compatibility, not Backend Capacity (thereby proving my point that not everyone knows what it means). :)

smiletrl’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

Thanks for the review!

  • I'm not sure what you mean at b) :(
  • For d), sorry for my English, this is something beyond my capacity.
  • Fro e), as @yched pointed out that , these paramters are lowercase, e.g., 'field_name'. If we change it into 'Field_name', it will be confused: is this what it should be in $values, $values['Feidl_name']?
  • Fro f), thanks for clarification! On the other hand, I see 'DX' has been used a lot in code:)

Status: Needs review » Needs work

The last submitted patch, document_field_and_instance-1968986-21.patch, failed testing.

smiletrl’s picture

Ok, I get it about b)

smiletrl’s picture

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

Status: Reviewed & tested by the community » Needs review
aspilicious’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -210,7 +210,29 @@ class Field extends ConfigEntityBase implements FieldInterface {
+	 *

Don't use tabs,use 2 spaces. You can configure your ide/text editor to do this automagicly!

aspilicious’s picture

Status: Needs review » Needs work
aspilicious’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -210,7 +210,22 @@ class Field extends ConfigEntityBase implements FieldInterface {
+   * ¶

Reviewed incorrect path but this doesn't need a trailing whitspace. :)

smiletrl’s picture

Thanks for the review!

smiletrl’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Novice, -Field API

The last submitted patch, document_field_and_instance-1968986-29.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
Issue tags: +Field API
jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Looks good to me! Thanks for all the hard work and iterations. I'll get this committed shortly.

amateescu’s picture

Maybe a RTBC before that would be nice? :P

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

OK I'll wait in case anyone else wants to review it. I was going to change the status to RTBC myself but forgot (I normally do that with docs patches, but I agree that for this issue perhaps it needs more review).

swentel’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -210,7 +210,22 @@ class Field extends ConfigEntityBase implements FieldInterface {
+   * In most cases, it's constructed via entity_create('field_entity', $values)), where

Exceeds 80 chars

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -201,7 +201,24 @@ class FieldInstance extends ConfigEntityBase implements FieldInstanceInterface {
+   *   elements will be used to set the corresponding properties on the class; see

Same here

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -201,7 +201,24 @@ class FieldInstance extends ConfigEntityBase implements FieldInstanceInterface {
+   * In most cases, it's constructed via entity_create('field_instance', $values)), where

Here as well

- edit - Other than that, looks good to me.

swentel’s picture

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

This should be good to go.

swentel’s picture

Hmm weird, dreditor and 80 chars don't seem to detect right ?!

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API

The last submitted patch, document_field_and_instance-1968986-38.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API
swentel’s picture

Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I took another look at this patch today...

+   * See: @link entity_create() @endlink.

How did that get overlooked before? Just use @see.

Also:

+   * In most cases, it's constructed via entity_create('field_entity', $values)),
+   * where $values is the same parameter for this constructor.

Can we rewrite this a bit? How about:
In most cases, Field entities are created via entity_create... same parameter as in this constructor.

jibran’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
3 KB

Made all the changes mentioned in #43.

jibran’s picture

FileSize
1.29 KB
3 KB

Changed @ingroup to field_crud on @swentel suggestion.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Great, thanks! I'll get this committed soon.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks all! Committed to 8.x.

smiletrl’s picture

Finally:) thanks!

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