As agreed in Prague (https://docs.google.com/document/d/1pgDY-RaTbajNqXm-oj8GdzPD_fPvLb2jhnrt...)
FieldDefinitionInterface is intended to be the "primary" interface for classes that implement it.
We should rename the methods from getFieldXxx() to just getXxx().

Also, badly needed after #2047229: Make use of classes for entity field and data definitions, which forces "FieldDefinitionInterface extends DataDefinitionInterface", meaning classes implementing FieldDefinitionInterface now need to provide both getFieldXxx() *and* getXxx() :-(

CommentFileSizeAuthor
#17 remove_field_prefix-2143263-17.patch258.12 KBplopesc
#13 remove_field_prefix-2143263-13.patch244.8 KBplopesc
#10 remove_field_prefix-2143263-10.patch242.44 KBplopesc
#8 remove_field_prefix-2143263-8.patch242.08 KBplopesc
#7 remove_field_prefix-2143263-7.patch240.44 KBplopesc
#5 remove_field_prefix-2143263-5.patch240.96 KBplopesc
#2 remove_field_prefix-2143263-2.patch226.59 KBplopesc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

yched CreditAttribution: yched commented
Issue tags: +Entity Field API, +Typed sanity

tagging

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Active » Needs review
FileSize
remove_field_prefix-2143263-2.patch226.59 KB

Attaching first approach.
Also removing prefix in setters defined in FieldDefinition to maintain consistency.

Let's see if testbot is green...

Regards.

Status: Needs review » Needs work

The last submitted patch, 2: remove_field_prefix-2143263-2.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented

Crossposting with #2047229: Make use of classes for entity field and data definitions, which was committed 5 min before :S

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review
FileSize
remove_field_prefix-2143263-5.patch240.96 KB

Re-roll

Status: Needs review » Needs work

The last submitted patch, 5: remove_field_prefix-2143263-5.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review
FileSize
remove_field_prefix-2143263-7.patch240.44 KB

New round...

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
FileSize
remove_field_prefix-2143263-8.patch242.08 KB

New re-roll

Status: Needs review » Needs work

The last submitted patch, 8: remove_field_prefix-2143263-8.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
FileSize
remove_field_prefix-2143263-10.patch242.44 KB

Fixing test fails in #8 and re-rolling again after #2148211: Use isSyncing flag to prevent creation of configuration entities on synchronisation and #2080671: Remove Unused local variable $key from /core/modules/field/lib/Drupal/field/Tests/Views/HandlerFieldFieldTest.php.
Let's see now...

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review

Go testbot!

Status: Needs review » Needs work

The last submitted patch, 10: remove_field_prefix-2143263-10.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review
FileSize
remove_field_prefix-2143263-13.patch244.8 KB

Let's see this time...
Really big patch, becomes obsolete even before testbot start to work...

Status: Needs review » Needs work

The last submitted patch, 13: remove_field_prefix-2143263-13.patch, failed testing.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs work » Needs review

13: remove_field_prefix-2143263-13.patch queued for re-testing.

yched’s picture

yched CreditAttribution: yched commented

@plopesc: if this breaks easily, we should probably push it in a branch in the D8 Field API sandbox (I granted you commit access). Would make rerolls easier.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
FileSize
remove_field_prefix-2143263-17.patch258.12 KB

New re-roll after some changes in 8.x branch.
Patch attached against f22264a43d951a1a668a6d280e9aaf70ecb2b510
This patch doesn't stop growing!
Regards

yched’s picture

yched CreditAttribution: yched commented

The patch mostly changes method names in :
\Drupal\Core\Field\FieldDefinitionInterface
\Drupal\Core\Field\FieldDefinition
\Drupal\field\Entity\Field
\Drupal\field\Entity\FieldInstance
The 200k+ rest of the patch is about modifying calling code accordingly.

I reviewed the four files above, and skimmed through the rest, and this looks good for me.

Before pushing RTBC, I'd like to make sure @fago and @effulgentsia validate this too, though. I'll ping them and ask if they can have a look quick-ish.

Meanwhile, since this patch is hell to maintain, I propose we freeze it at the green patch in #17 (has a 8.x commit hash on which it applies and works), and stop trying to reroll for now. When we reach the "go" phase, we reroll and sync with a core committer to get a blitz commit as soon as we reach green again.

Again, thanks a lot for taking this, @plopesc !

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I only looked at the 4 files mentioned in #18, but those look great! +1 to yched RTBC'ing when he feels it's ready.

The initial reason for the Field prefix was back when config entities had conflicting method names (e.g., isTranslatable() referring to entity translation, not field translation). However, now that config entities have been pared back from that, having those names available for the business logic of fields is awesome.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

I reviewed the important places, and quickly looked over all the replacements - looks all good to me. +1 to move on with this as well.

+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -78,7 +78,7 @@ protected function getFieldSettings() {
   protected function getFieldSetting($setting_name) {
-    return $this->getFieldDefinition()->getFieldSetting($setting_name);
...
   }

Imho this method should become getSetting() also, but that can totally be a follow-up - this is big enough already ;-)

yched’s picture

yched CreditAttribution: yched commented

#20: Agreed that we should consider that, and agreed that it's probably better in a separate issue :-).

Thanks folks for chiming in so quick !

I'll try to grab a core committer to agree on a commit window so that we don't waste too much time in rerolls.

yched’s picture

yched CreditAttribution: yched commented

Created #2152825: rename FieldItemBase::getFieldSetting[s]() to getSetting[s]()

yched’s picture

yched CreditAttribution: yched commented

17: remove_field_prefix-2143263-17.patch queued for re-testing.

yched’s picture

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

Green !
Go go go go go !

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Remove "Field" prefix from FieldDefinitionInterface methods » Change notice: Remove "Field" prefix from FieldDefinitionInterface methods
Status: Reviewed & tested by the community » Active
Issue tags: +API change, +Approved API change, +Needs change record

Committed cdac25e and pushed to 8.x. Thanks!

Need to update any relevant change notices.

yched’s picture

yched CreditAttribution: yched commented
Title: Change notice: Remove "Field" prefix from FieldDefinitionInterface methods » Remove "Field" prefix from FieldDefinitionInterface methods
Status: Active » Fixed
Issue tags: -Needs change record

Thanks a lot for being so quick Alex.

- Updated https://drupal.org/node/2090627
- #2047229: Make use of classes for entity field and data definitions has no change notice yet
- Checked the notices for "formatters as plugins", "widgets as plugins", "field types as plugins", 'configurable fields as ContentEntities", and they didn't need updates.

Status: Fixed » Closed (fixed)

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