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() :-(
Comment | File | Size | Author |
---|---|---|---|
#17 | remove_field_prefix-2143263-17.patch | 258.12 KB | plopesc |
#13 | remove_field_prefix-2143263-13.patch | 244.8 KB | plopesc |
#10 | remove_field_prefix-2143263-10.patch | 242.44 KB | plopesc |
#8 | remove_field_prefix-2143263-8.patch | 242.08 KB | plopesc |
#7 | remove_field_prefix-2143263-7.patch | 240.44 KB | plopesc |
Comments
Comment #1
yched CreditAttribution: yched commentedtagging
Comment #2
plopescAttaching first approach.
Also removing prefix in setters defined in FieldDefinition to maintain consistency.
Let's see if testbot is green...
Regards.
Comment #4
plopescCrossposting with #2047229: Make use of classes for entity field and data definitions, which was committed 5 min before :S
Comment #5
plopescRe-roll
Comment #7
plopescNew round...
Comment #8
plopescNew re-roll
Comment #10
plopescFixing 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...
Comment #11
plopescGo testbot!
Comment #13
plopescLet's see this time...
Really big patch, becomes obsolete even before testbot start to work...
Comment #15
plopesc13: remove_field_prefix-2143263-13.patch queued for re-testing.
Comment #16
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.
Comment #17
plopescNew re-roll after some changes in 8.x branch.
Patch attached against f22264a43d951a1a668a6d280e9aaf70ecb2b510
This patch doesn't stop growing!
Regards
Comment #18
yched CreditAttribution: yched commentedThe 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 !
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #20
fagoI reviewed the important places, and quickly looked over all the replacements - looks all good to me. +1 to move on with this as well.
Imho this method should become getSetting() also, but that can totally be a follow-up - this is big enough already ;-)
Comment #21
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.
Comment #22
yched CreditAttribution: yched commentedCreated #2152825: rename FieldItemBase::getFieldSetting[s]() to getSetting[s]()
Comment #23
yched CreditAttribution: yched commented17: remove_field_prefix-2143263-17.patch queued for re-testing.
Comment #24
yched CreditAttribution: yched commentedGreen !
Go go go go go !
Comment #25
alexpottCommitted cdac25e and pushed to 8.x. Thanks!
Need to update any relevant change notices.
Comment #26
yched CreditAttribution: yched commentedThanks 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.