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()
, andfield_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.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#45 | 1968986-45.patch | 3 KB | jibran |
#45 | interdiff.txt | 1.29 KB | jibran |
#44 | document_field_and_instance-1968986-44.patch | 3 KB | jibran |
#44 | interdiff.txt | 2.16 KB | jibran |
#38 | document_field_and_instance-1968986-38.patch | 2.97 KB | swentel |
Comments
Comment #1
xjmComment #2
yched CreditAttribution: yched commentedUn-postponing.
Comment #3
smiletrl CreditAttribution: smiletrl commentedThis 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.,
So, I guess it will be redundant to type them again. Only identify whether this property is required or not in constructor doc atm.
Comment #4
smiletrl CreditAttribution: smiletrl commentedComment #5
swentel CreditAttribution: swentel commentedGood start, but a lot of tabs in the patch.
Comment #6
smiletrl CreditAttribution: smiletrl commentedThis patch contaings both field and field instance. I guess, some properties should not be provided via constructor variables. Like
class FieldInstance
:Anyway, include them in this doc atm.
Comment #7
jhodgdonThanks! 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):
b) Both of the constructors are missing @param $entity_type documentation.
c) This English wording needs a fix:
Should be "For specific property descriptions, please refer to the property documentation inside this class."
d) This English wording needs a fix:
Should be "Defaults to 1.".
e)
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.
Comment #8
yched CreditAttribution: yched commented+ * - 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 ?
Comment #9
smiletrl CreditAttribution: smiletrl commentedThanks for the review!
For
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,$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:)
Comment #10
smiletrl CreditAttribution: smiletrl commentedComment #11
smiletrl CreditAttribution: smiletrl commentedJust for the records, at the top parent Class Entity::__construct function, every member variable gets a chance to be initialized by
$values
array.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.
Comment #12
yched CreditAttribution: yched commentedShould be 'type'. The actual property names are lowercased, we can't uppercase them in docs.
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'
Comment #13
smiletrl CreditAttribution: smiletrl commentedThanks 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()
. Whilefield_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 offield_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 ofentity_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');
.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:)Comment #14
jhodgdonThe 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
Comment #15
smiletrl CreditAttribution: smiletrl commentedDiscussed 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^^
Comment #16
smiletrl CreditAttribution: smiletrl commentedComment #17
smiletrl CreditAttribution: smiletrl commentedWrong word *detials* at #15.
Comment #18
jhodgdonThanks 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)).
Comment #19
smiletrl CreditAttribution: smiletrl commentedThanks for review!
New patch.
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.
Comment #20
smiletrl CreditAttribution: smiletrl commentedComment #21
jhodgdonThanks 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
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). :)
Comment #22
smiletrl CreditAttribution: smiletrl commentedThanks for the review!
Comment #24
smiletrl CreditAttribution: smiletrl commentedOk, I get it about b)
Comment #25
smiletrl CreditAttribution: smiletrl commentedComment #26
smiletrl CreditAttribution: smiletrl commentedComment #27
aspilicious CreditAttribution: aspilicious commentedDon't use tabs,use 2 spaces. You can configure your ide/text editor to do this automagicly!
Comment #28
aspilicious CreditAttribution: aspilicious commentedComment #29
aspilicious CreditAttribution: aspilicious commentedReviewed incorrect path but this doesn't need a trailing whitspace. :)
Comment #30
smiletrl CreditAttribution: smiletrl commentedThanks for the review!
Comment #31
smiletrl CreditAttribution: smiletrl commentedComment #33
smiletrl CreditAttribution: smiletrl commented#30: document_field_and_instance-1968986-29.patch queued for re-testing.
Comment #34
jhodgdonLooks good to me! Thanks for all the hard work and iterations. I'll get this committed shortly.
Comment #35
amateescu CreditAttribution: amateescu commentedMaybe a RTBC before that would be nice? :P
Comment #36
jhodgdonOK 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).
Comment #37
swentel CreditAttribution: swentel commentedExceeds 80 chars
Same here
Here as well
- edit - Other than that, looks good to me.
Comment #38
swentel CreditAttribution: swentel commentedThis should be good to go.
Comment #39
swentel CreditAttribution: swentel commentedHmm weird, dreditor and 80 chars don't seem to detect right ?!
Comment #41
swentel CreditAttribution: swentel commented#38: document_field_and_instance-1968986-38.patch queued for re-testing.
Comment #42
swentel CreditAttribution: swentel commentedComment #43
jhodgdonI took another look at this patch today...
How did that get overlooked before? Just use @see.
Also:
Can we rewrite this a bit? How about:
In most cases, Field entities are created via entity_create... same parameter as in this constructor.
Comment #44
jibranMade all the changes mentioned in #43.
Comment #45
jibranChanged
@ingroup
tofield_crud
on @swentel suggestion.Comment #46
swentel CreditAttribution: swentel commentedGreat, thanks!
Comment #47
jhodgdonGreat, thanks! I'll get this committed soon.
Comment #48
jhodgdonThanks all! Committed to 8.x.
Comment #49
smiletrl CreditAttribution: smiletrl commentedFinally:) thanks!