Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
5.45 KB

Initial patch

yched’s picture

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
@@ -218,7 +220,7 @@ public function getFieldMap() {
+   * @return \Drupal\field\FieldInterface[]

Never saw that syntax before, but it seems we start to use it in other places in core... Is that an offical doc standard now ?

yched’s picture

Status: Needs review » Reviewed & tested by the community

Wow, Phpstorm actually recognizes this and provides method completion accordingly, that's pretty awesome :-)

andypost’s picture

@yched yes same here, but dawehner suggested this syntax

andypost’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.82 KB

prepareExtraFields() is gone
Also removed useless

use Drupal\field\FieldInterface;
use Drupal\field\FieldInstanceInterface;

because they are in the same namespace

yched’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
    @@ -256,10 +256,10 @@ public function getFields() {
    -   * @return
    +   * @return \Drupal\field\FieldInstanceInterface[]
        *   If $entity_type is not set, all instances keyed by entity type and bundle
        *   name. If $entity_type is set, all instances for that entity type, keyed
        *   by bundle name.
    

    FieldInstanceInterface[] is not correct, unfortunately, see the description of the @return . The current (arguably bad DX) behavior of the function is to have different return structure depending on the params

  2. +++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
    @@ -508,7 +508,7 @@ public function getBundleInstances($entity_type, $bundle) {
    +   * @return \Drupal\field\FieldInstanceInterface|NULL
        *   An associative array of instance data for the specific field and bundle;
        *   NULL if the instance does not exist.
    

    The description for the @return is totally invalid now - an $instance structure is not an associative array anymore. Could we fix that while we're in here ?

andypost’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
1.07 KB

Nice catch!

yched’s picture

Thanks for being so quick !
Sorry, actually the main description for the getInstance() method is similarly badly outdated :-/

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
@@ -509,7 +509,7 @@ public function getBundleInstances($entity_type, $bundle) {
+   *   The instance definition for the specific field of the entity bundle;

Awkward phrasing. Maybe just "The field instance definition, or NULL if it does not exist" ?

andypost’s picture

@yched you awesome as always!

andypost’s picture

yched> andypost: the first line of the phpdoc for getInstance() should be fixed as well :-)
yched> andypost: "Returns an array of instance data..." -> "Returns a field instance definition."

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yay :-)
Thanks !

The last submitted patch, drupal8.field-system.2092265-12.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +Field API
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Some strange failure of upgrade tests has happen

andypost’s picture

Issue tags: +Documentation

tagging

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

Xano’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.27 KB

Re-roll, and I fixed some more type hints in code comments. I just don't know how to interdiff if there's a patch that no longer applies.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -DX (Developer Experience), -Field API
FileSize
4.64 KB

Here it is.

amateescu’s picture

  1. +++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
    @@ -344,10 +344,10 @@ public function getField($entity_type, $field_name) {
    -   * @return
    +   * @return \Drupal\field\FieldInterface|NULL
    

    NULL -> null in typehints.

  2. +++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
    @@ -555,13 +556,12 @@ public function getBundleExtraFields($entity_type, $bundle) {
    -   * @return
    -   *   The field definition completed for the current runtime context.
    +   * @return \Drupal\field\FieldInterface
    
    @@ -571,13 +571,12 @@ public function prepareField(FieldInterface $field) {
    -   * @return
    -   *   The field instance array completed for the current runtime context.
    +   * @return \Drupal\field\FieldInstanceInterface
    

    What's the reason for removing the description text?

I'd say #20 is RTBC, not #19 :)

Xano’s picture

Assigned: Unassigned » Xano
Status: Reviewed & tested by the community » Needs work

#20 forgets to update a few descriptions that still mention array structures.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
3.33 KB
5.74 KB
andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC

webchick’s picture

Component: field system » documentation
Status: Reviewed & tested by the community » Fixed
Issue tags: -Documentation

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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