Problem/Motivation

The doc blocks for configureEntityFormDisplay() and configureEntityViewDisplay() are inaccurate.

Originally, there was just one method, configureEntityDisplays(). In #2446511: Add a "preconfigured field options" concept in Field UI this was split up into two methods, and the original doc block was copied for both. The parameter comments were updated, but the first line of the doc block still reads

   * Configures the newly created field for the default view and form modes.

even though one of the new functions handles view modes and the other handles form modes.

Proposed resolution

Update the doc blocks to describe accurately what each method does.

Remaining tasks

  • Update the doc blocks. (done)
  • Apply the patch and make sure that it applies correctly.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#17 2847685-17.patch1.33 KBtameeshb
#5 update_doc_blocks_for-2847685-5.patch888 bytessemiuniversal
#4 update_doc_blocks_for-2847685-4.patch888 bytessemiuniversal

Comments

benjifisher created an issue. See original summary.

semiuniversal’s picture

semiuniversal commented

I'll take a look at this.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue tags: +SprintWeekend2017, +SprintWeekendBOS
semiuniversal’s picture

semiuniversal commented
StatusFileSize
new update_doc_blocks_for-2847685-4.patch888 bytes

OK, here's a patch that edits the 2 doc blocks.
I noticed that not only was the method split up into 2 methods, but that it was used for both newly-created and existing fields. Therefore I and removed the "newly created" language from the documentation.

semiuniversal’s picture

semiuniversal commented
StatusFileSize
new update_doc_blocks_for-2847685-5.patch888 bytes

Oops, I uploaded the wrong patch file. Here's the right one.

mirie’s picture

mirie commented
Status: Active » Needs review
mirie’s picture

mirie commented

Doc comment changes in patch (update_doc_blocks_for-2847685-5.patch) look good to me!

mirie’s picture

mirie commented
Status: Needs review » Reviewed & tested by the community
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

Good point about removing "newly created" from the doc comments. The lines

        $this->configureEntityFormDisplay($values['field_name'], $widget_id);
        $this->configureEntityViewDisplay($values['field_name'], $formatter_id);

appear twice in submitForm(): once for newly created fields, once for reused fields.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.3.x-dev » 8.2.x-dev

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

cilefen commented

Updating issue credit.

  • cilefen committed 378b688 on 8.4.x 
    Issue #2847685 by semiuniversal, benjifisher: Update doc blocks for...

  • cilefen committed 935d14d on 8.4.x 
    Revert "Issue #2847685 by semiuniversal, benjifisher: Update doc blocks...
cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work

I reverted this because when the patch applies, the comments are applied to the wrong respective functions. Yes, I'm serious.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue tags: +Novice

Wow, I have never seen that happen before. I tried it myself, using the current HEAD of the 8.4.x branch, and this is what happened:

$ git apply -v --index /tmp/update_doc_blocks_for-2847685-5.patch
Checking patch core/modules/field_ui/src/Form/FieldStorageAddForm.php...
Hunk #1 succeeded at 428 (offset 6 lines).
Hunk #2 succeeded at 410 (offset -30 lines).
Applied patch core/modules/field_ui/src/Form/FieldStorageAddForm.php cleanly.

I guess when git (or patch) looks at the first hunk, it prefers an offset of +6 to one of -12, and it is not smart enough to look ahead at the second hunk.

It should be pretty easy to re-do the patch, so I will add the "Novice" tag.

To make sure we do not get the same problem, let's study the available options for git diff ... I think this will work:

git diff -U6

to provide 6 lines of context instead of the default 3.

For reference, other options we could use include --inter-hunk-context=12 or --function-context (short form -W).

tameeshb’s picture

tameeshb commented
Status: Needs work » Needs review
StatusFileSize
new 2847685-17.patch1.33 KB

Tried with

git diff -U6
tameeshb@imt3rabyt3 /v/w/h/drupal84> git diff -U6 8.4.x> 2847685-17.patch
tameeshb@imt3rabyt3 /v/w/h/drupal84> git checkout 8.4.x
Switched to branch '8.4.x'
Your branch is up-to-date with 'origin/8.4.x'.
tameeshb@imt3rabyt3 /v/w/h/drupal84> git apply --check 2847685-17.patch 
tameeshb@imt3rabyt3 /v/w/h/drupal84>
tameeshb’s picture

tameeshb commented

Applied to a new branch and verified.

  /**
   * Configures the field for the default form mode.
   *
   * @param string $field_name
   *   The field name.
   * @param string|null $widget_id
   *   (optional) The plugin ID of the widget. Defaults to NULL.
   */
  protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {
    // Make sure the field is displayed in the 'default' form mode (using
    // default widget and settings). It stays hidden for other form modes
    // until it is explicitly configured.
    $options = $widget_id ? ['type' => $widget_id] : [];
    entity_get_form_display($this->entityTypeId, $this->bundle, 'default')
      ->setComponent($field_name, $options)
      ->save();
  }

  /**
   * Configures the field for the default view mode.
   *
   * @param string $field_name
   *   The field name.
   * @param string|null $formatter_id
   *   (optional) The plugin ID of the formatter. Defaults to NULL.
   */
  protected function configureEntityViewDisplay($field_name, $formatter_id = NULL) {
    // Make sure the field is displayed in the 'default' view mode (using
    // default formatter and settings). It stays hidden for other view
    // modes until it is explicitly configured.
    $options = $formatter_id ? ['type' => $formatter_id] : [];
    entity_get_display($this->entityTypeId, $this->bundle, 'default')
      ->setComponent($field_name, $options)
      ->save();
  }
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes
Issue tags: +Baltimo2017

@tameeshb, thanks for updating the patch!

I think this is still a novice task.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue tags: -Baltimo2017 +Baltimore2017

Fixing the tag ...

ohthehugemanatee’s picture

ohthehugemanatee commented
Status: Needs review » Reviewed & tested by the community

Tested. Patch applies cleanly, docblocks are on the right functions.

  • cilefen committed 9bdb51c on 8.4.x 
    Issue #2847685 by semiuniversal, tameeshb, benjifisher: Update doc...

  • cilefen committed d444aad on 8.3.x 
    Issue #2847685 by semiuniversal, tameeshb, benjifisher: Update doc...
cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Fixed

Committed 9bdb51c and pushed to 8.4.x. Backported to 8.3.x as d444aad because docs improvements are allowed in patch releases. Thanks!

Status: Fixed » Closed (fixed)

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