Problem/Motivation

Looks like #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods went in with a couple of missing return statements.

Proposed resolution

Add return $this to FieldConfigBase::setSetting(), FieldConfigBase::setConstraints(), and FieldConfigBase::addConstraint().

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 2589829-5.patch880 bytesrakesh.gectcr
#2 2589829-2.patch643 bytesjoshi.rohit100

Comments

Arla created an issue. See original summary.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Status: Active » Needs review
StatusFileSize
new 2589829-2.patch643 bytes
yched’s picture

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

Looks good, thanks

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2589829-2.patch, failed testing.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review
StatusFileSize
new 2589829-5.patch880 bytes
yched’s picture

yched commented
Issue tags: +rc target triage

Thanks @rakesh.gectcr. So addConstraint() is impacted too ? Can we add it to the issue title and summary ?

I'm not where I can check the interface atm, so not setting to RTBC myself, anyone feel free.

Also, this should be ok to get in during RC phase, since it's fixing an incorrect interface implementation, and will not cause any code break. Adding the triage tag to let committers decide.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Issue summary: View changes
rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented

@yched

I have added that into issue summary.

swentel’s picture

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

Yep, addConstraint wants it back to.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -rc target triage +rc target

Discussed with @alexpott, @effulgentsia, and @catch. This is mostly a coding standards improvement based on: https://www.drupal.org/node/608152#chaining

This does change the return values of these functions, but I can't really imagine that any code could possibly need to rely on the fact that these methods always return NULL in HEAD.

I also checked and confirmed that all three methods already have documentation that the method returns $this. So committed and pushed to 8.0.x. Thanks!

  • xjm committed 23789d0 on 8.0.x 
    Issue #2589829 by rakesh.gectcr, joshi.rohit100, yched, Arla: Missing...

Status: Fixed » Closed (fixed)

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