Follow up from #1735118: Convert Field API to CMI
Convert array syntax to object syntax in existing code, add type hints for $field & $instance arguments across core functions, update docs.
Commit message:
Issue #1953408 by swentel, aspilicious, andypost, yched: Remove ArrayAccess BC layer from field config entities.
Conversion happens in sandbox
Log of commits
Helper issue is over at #2013679: Helper issue for #1953408 (Remove ArrayAccess BC layer)
API changes
Remove ArrayAccess from Fields and Instance object and replace all array access to object property access.
Direct mapping for most properties:
$instance['label'] -> $instance->label
...
Special cases:
- $field['id'] -> $field->uuid (means renaming a couple $field_id variables for clarity...)
- $field['field_name'] -> $field->name (after #1497374: Switch from Field-based storage to Entity-based storage, ->id() right now)
- $field['columns'] -> $field->getFieldPropertyNames()
- $field['foreign keys'] -> $schema = $field->getSchema(); $schema['foreign keys'];
- $field['bundles'] -> $field->getBundles()
- $instance['field_id'] -> $instance->field_uuid (+ $field_id renames)
- $instance['field_name'] -> $instance->getFieldName()
Note:
This is technically an API break, but the support for the old "array access" syntax has been explicitly mentioned as "temporary, will be removed" in the change notice for "Convert Field API to CMI".
Also, the array access layer has a performance impact (hard to evaluate precisely without writing the actual patch...)
Comment | File | Size | Author |
---|---|---|---|
#63 | interdiff.txt | 914 bytes | swentel |
#63 | 1953408-63.patch | 245.98 KB | swentel |
#60 | 1953408-60.patch | 245.09 KB | swentel |
#60 | interdiff.txt | 2.45 KB | swentel |
#53 | 1953408-53.patch | 249.92 KB | aspilicious |
Comments
Comment #1
swentel CreditAttribution: swentel commentedtagging
Comment #2
yched CreditAttribution: yched commentedI think we should tackle #1967106: FieldInstance::getField() method first. Then, the patch in this issue will naturally go over all the places where that new $instance::getField() method makes more fluent code.
Easier than the other way around (move all code to the new object syntax, then go over it all again to find the places where $instance::getField() makes sense)
Comment #3
yched CreditAttribution: yched commentedThus, keeping this one postponed on #1967106: FieldInstance::getField() method
Comment #4
andypostThis would be a second pass after #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API to fix a lot of tests so let's decide on strategy to review it.
Comment #5
yched CreditAttribution: yched commented@andypost: I don't understand what you mean.
What kind of strategy do you feel is missing here, for what kind of concerns ?
Comment #6
andypostI mean that tests will need a lot of work and get a lot of collisions with #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest
So better join forces and initially refactor tests in separate issues on per-file or per-functionality base
Comment #7
yched CreditAttribution: yched commentedRight, waiting on #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest to be sorted out before we mess with the same files makes sense to me.
This issue is already postponed anyway, but adding that to the summary.
Comment #8
xjmLet's go with meta. :) Also, this is release-blocking I think?
Comment #9
swentel CreditAttribution: swentel commentedWe agreed doing this in small chunks.
Comment #10
BerdirAs this is a meta issue now, seems like a lot can already be done outside the files touched in that issue and especially once #2004224: Convert tests using TestEntity to EntityTest (except Field API tests) is in, no?
Unpostponing...
Comment #11
swentel CreditAttribution: swentel commentedCreated a branch in the sandbox 'field-remove-BC' - will start on it slowly and we'll create additional issues like we've done in #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API
Comment #11.0
swentel CreditAttribution: swentel commentedLink to issues this depends on
Comment #11.1
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #11.2
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #12
swentel CreditAttribution: swentel commented@yched in case you read this.
There's a lot of places where we have instance['field_name']. Now we could do $instance->getField()->id() then of course, however, wouldn't it be interesting to have a $instance->getFieldName() as well. Looks a bit more sane to me.
Comment #13
yched CreditAttribution: yched commentedYeah, $instance->getField()->id() is the current way, and I agree it's not too great.
+1 on a dedicated method. $instance->getFieldId(), then ?
Comment #13.0
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #14
yched CreditAttribution: yched commentedre #12 / 13: #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets has added FieldInstance::getFieldName() since then, so this part is solved :-)
Added "API changes" considerations in the OP.
Comment #14.0
yched CreditAttribution: yched commentedAPI changes
Comment #15
yched CreditAttribution: yched commented@swentel : is this what the field-remove-BC sandbox is about ? Do you have a "status estimation" on what's in there ?
Comment #16
alexpottTagging to approve the API changes necessary to remove the BC layer. The new API has already landed. We need to convert everything to it and remove the BC layer. The removal was announced pre code freeze.
Comment #17
swentel CreditAttribution: swentel commentedStatus update: aspilicious, andypost, pcambra and me have been working on this one periodically, but it's been a while since we merged HEAD on the field-remove-BC branch in the sandbox. I'll kickstart this again next week during drupalaton.
Comment #17.0
swentel CreditAttribution: swentel commentedduplicate header
Comment #18
yched CreditAttribution: yched commented@swentel: thanks !
- I added a note in the OP about renaming $field_id variables when they now receive a UUID
- is this also the issue where we properly typehint FieldInterface / FieldInstanceInterface in function params ?
Comment #19
swentel CreditAttribution: swentel commented@yched yeah I'm typehinting them, see helper issue already.
Comment #20
swentel CreditAttribution: swentel commentedFirst patch to get reviews on. Given the size, I think it's doable to get this in in one move. I've added the suggested commit message to the IS as well (please update in case I missed someone).
Comment #20.0
swentel CreditAttribution: swentel commentedrename of $field_id variables
Comment #21
andypostI think we should update summary with 2 columns: field and instance
Also usage of settings needs to be cleared according #2074217: Reconsider support for field-level & instance-level settings with the same name ? because some places use
getFieldSetting*()
but others needs direct access to instance (image field for example)Comment #22
yched CreditAttribution: yched commented@andypost: let's not mix issues please. The special case of image field that has to do tricky dance to access its 'default_image' in either the $field or the $instance is discussed at #2074217: Reconsider support for field-level & instance-level settings with the same name ?. It's an edge case.
The motto is:
- code that works on a generic FieldDefinitionInterface (widgets, formatters, field type classes) needs to access settings through ->getSettings(), which returns the "merged" settings.
- code that specifically handles configurable $fields & $instances (housekeeping code in field.module, field_ui...) needs to work with $field->settings and $instance->settings.
Comment #23
aspilicious CreditAttribution: aspilicious commentedKeeping up with core. If this one fails I'm going back to the helper issue :)
Comment #24
aspilicious CreditAttribution: aspilicious commentedWrong patch
Comment #25
aspilicious CreditAttribution: aspilicious commentedreupload...
Comment #27
aspilicious CreditAttribution: aspilicious commentedreroll hell
Comment #29
andypostFiled 2 follow-ups according #2013679-186: Helper issue for #1953408 (Remove ArrayAccess BC layer)
#2087429: Remove deprecated call field_info_instance() in ImageFormatterBase.php
#2087449: Fix broken tests FieldAttachOtherTest->testFieldAttachCache()
Comment #30
andypostanother round of merge
Comment #31
andypostComment #32
andypostCleaned up
ImageFormatterBase
according #2075095: Widget / Formatter methods signatures needlessly complexSuppose this patch is ready
Comment #33
Berdir$field->storage?
Isn't that gone now? Are we missing test coverage here? The condition can just be removed I assume?
A bit strange that we added getFieldLabel() as an alias for label(). I can understand that we added getTitle() on NodeInterface but both methods use label(). Not relevant..
Interesting, looks like this triggers a bug in git diff --color-words. There, $field->save() is completely green, looks like that was added.
This puts the getFieldSetting() on a separate line because isset($field->getFieldSetting()) doesn't work, but isset($variable) on a variable that was set with $variable = .. on the line above is unecessary.
If you want to check for NULL, why not use assertNull($field->getFieldSetting()) ? That does assert(!isset() internally, which means it's the same as the current code.
Comment #34
andypostaddressed 3 and 4
Comment #36
yched CreditAttribution: yched commented@Berdir #33.1 : this is fixed in #2086095: Remove remaining references to field_sql_storage, with tests
Comment #37
andypostcurrent sandbox
Comment #38
swentel CreditAttribution: swentel commentedrerolled
Comment #39
Berdir#38: 1953408-38.patch queued for re-testing.
Comment #41
aspilicious CreditAttribution: aspilicious commentedReroll...
Comment #43
yched CreditAttribution: yched commentedPushed a reroll to the sandbox
Comment #44
aspilicious CreditAttribution: aspilicious commentedLets see what happens now...
Comment #45
aspilicious CreditAttribution: aspilicious commentedAnother update
Comment #47
aspilicious CreditAttribution: aspilicious commentedFixed some issues
Comment #49
aspilicious CreditAttribution: aspilicious commentedMèèh
Comment #51
aspilicious CreditAttribution: aspilicious commentedShould kill the exceptions
Comment #53
aspilicious CreditAttribution: aspilicious commentedLets try this one
Comment #54
yched CreditAttribution: yched commentedReviewed this in Prague, seen a couple adjustments with @swentel and @aspilicious, this is ready for me.
Thanks a lot @swentel, @andypost, @aspilicious for keeping this patch rolling. Let's get this in !
Comment #55
yched CreditAttribution: yched commentedEr, sorry :-(
Not sure I'm too keen on that :/
At the very least, the default value should be NULL, not ''.
Then, the @return phpdoc section for FieldInterface::getSchema() is now incomplete, since the returned array might be a full or a partial schema depending on the $key param.
Functions whose return structure varies widely depending on a param are not a great pattern IMO. We have some in core, the goal should be to remove them, not to add new ones.
In the end, this is just a helper to workaround the minor annoyance of PHP 5.3 not allowing $object->mehtod()['key'] - which is fixed in 5.4.
I'd rather not add confusions to the APIs for this.
In HEAD, $field['id'] is actually the UUID, so the end of that line should be
|| ($field && $field->uuid() != $current_field->uuid())) {
Comment #56
yched CreditAttribution: yched commentedComment #57
swentel CreditAttribution: swentel commented@yched how about adding getSchemaKey() as an extra method then ?
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedJust quickly searching for getSchema in #53, I don't see where it's called with a $key parameter. What am I missing?
Comment #59
swentel CreditAttribution: swentel commentedHmm good question, will look at it.
Rerolling at the moment too, there are some conflicts, should get it in half an hour or so.
Comment #60
swentel CreditAttribution: swentel commentedSo yeah, we don't need it anymore, the entity base storage patch made it obsolete.
Addressed .2 as well - I think we're good to go again ?
Comment #61
yched CreditAttribution: yched commentedThanks! Works for me if green.
Comment #63
swentel CreditAttribution: swentel commentedOh man
Comment #63.0
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #64
alexpottCommitted bcc5763 and pushed to 8.x. Thanks!
I had to rebase the patch because it no longer applied :)
But git took care of the merge.
Comment #65
swentel CreditAttribution: swentel commentedUpdated https://drupal.org/node/2012896 which mentioned the temporary array access layer. I think we're good here.
Comment #66
swentel CreditAttribution: swentel commentedRemoving tags
Comment #67.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #68
yched CreditAttribution: yched commentedSee #2134861: Field instance bundle is only accessible directly on the bundle property