Problem/Motivation

There's a lot of code repetition in #2893073: Create a EntityReference Field plugin, #2893075: Create a ListField Field plugin, #2893076: Create a NumberField Field plugin, #2893077: Create a PhoneField Field plugin

Proposed resolution

As mentioned in #2893073-8: Create a EntityReference Field plugin, let's add a default action for processFieldValues() & getFieldFormatterMap().

Remaining tasks

  • Write a patch
  • Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#16 interdiff-10-16.txt765 bytesmaxocub
#16 2896507-16.patch10.07 KBmaxocub
#10 2896507-9.patch9.33 KBheddn
#9 2896507-8.patch0 bytesheddn
#9 interdiff_3-8.txt1.53 KBheddn
#3 2896507-3.patch8.88 KBheddn
#2 2896507.patch851 bytesheddn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2896507.patch851 bytes
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Issue summary: View changes
FileSize
2896507-3.patch8.88 KB

@maxocub: The FieldPluginBase also doesn't implements the getFieldFormatterMap() method, so we will still need those and not only the annotations. Should we also add it to the base class while we're at it?

I've also cleaned up implementations of child classes where they were already implementing the empty functionality. No interdiff.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Title: Update FieldPluginBase with a default processFieldValues() » Update FieldPluginBase with a default processFieldValues() and getFieldFormatterMap()
maxocub’s picture

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

Nice work cleaning it up!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2896507-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs work » Reviewed & tested by the community

Failure in unrelated part of code. Back to rtb.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2896507-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff_3-8.txt1.53 KB
2896507-8.patch0 bytes

Updated patch to fix code standards caught by test bot for unused imports. Back to RTBC and hopes that the unrelated test failures don't cause additional false failures.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
FileSize
2896507-9.patch9.33 KB

Oops

The last submitted patch, 9: 2896507-8.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2896507-9.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs work » Reviewed & tested by the community

Stop it.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Let's do it, but this needs a change record.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record
FileSize
2896507-16.patch10.07 KB
interdiff-10-16.txt765 bytes

Created a change record draft.
Also removed an implementation of getFieldFormatterMap() that got in since the last patch.
My change was trivial, so I take a chance and put it directly back to RTBC.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented

+1 on rtbc

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented

Also did another review of the CR. All looks good here.

  • catch committed a47d8c4 on 8.5.x 
    Issue #2896507 by heddn, maxocub: Update FieldPluginBase with a default...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +8.4.0 release notes

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Also tagging for release notes.

Status: Fixed » Closed (fixed)

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

maxocub’s picture

maxocub CreditAttribution: maxocub for Acquia commented
maxocub’s picture

maxocub CreditAttribution: maxocub as a volunteer and for Acquia commented