Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#30 | 1981306-30.patch | 49.06 KB | amateescu |
#27 | 1981306-27-sandbox-do-not-test.patch | 12.53 KB | pcambra |
#26 | 1981306-26.patch | 49.07 KB | pcambra |
#24 | interdiff.txt | 39.09 KB | andypost |
#24 | 1981306-24.patch | 47.39 KB | andypost |
Comments
Comment #1
andypostComment #2
andyposttaggin
Comment #3
andyposttagged to office hours
Comment #4
aspilicious CreditAttribution: aspilicious commentedComment #5
swentel CreditAttribution: swentel commentedWe're going todo this in one patch.
Comment #6
swentel CreditAttribution: swentel commentedSo we agreed to this in chunks anyway.
Comment #7
swentel CreditAttribution: swentel commented#1: 1953410-cud-1981306-1.patch queued for re-testing.
Comment #9
swentel CreditAttribution: swentel commentedrerolled
Comment #10
swentel CreditAttribution: swentel commentedThis looks pretty neat to me, nearly RTBC, apart from one that I'm not sure of. Assigning to yched for opinion.
Should we do entity_load('id')->delete() here ?
Comment #11
yched CreditAttribution: yched commentedre #10: Unless I'm missing something, I don't think it makes an actual difference, in the end it's the same object that gets deleted.
field_info_field() is still the performance-friendly way to access a Field entity, so it's probably still the one we should promote vs. entity_load(), although both will work here.
Only nitpicks below, I'll let you judge whether you think this is worth fixing.
(+ other places down in the method)
$this->field is now a Field entity rather than an array, so technically this still works thank to the ArrayAccess BC, and we have a separate issue to get rid of the BC, but strictly speaking this is adding a *new* use of the BC, which we might want to avoid ?
(meaning, depending on the order of patches, it might slip through the cracks and not get removed)
Your call :-)
Same here, and in the testViewsData() method lower.
Same
No biggie, might be clearer to ditch the array variables and have the rest of the test work on the Entities, with object syntax ?
(same in EntityFormDisplayTest)
The $field and $instance arrays are not needed, could be inlined.
Same for the $field_info / $instance_info arrays here.
Comment #12
swentel CreditAttribution: swentel commentedHuh ? I think whoever comes first, that will be removed anyway no ? Or am I missing something completely ? Of course, it might probably be easier todo that here, because it's going to clash like hell of course .. :/
Comment #13
yched CreditAttribution: yched commentedYeah, dunno what would exactly happen - since those are places that currently don't rely on the ArrayAccess BC, chances are they might not be affected by the other patch, so there would be no conflict... Might be tedious or not.
But yeah, point is, we might want to avoid introducing new uses of the BC layer in places that currently don't use it.
Comment #14
aspilicious CreditAttribution: aspilicious commentedLet's try this one
Comment #16
aspilicious CreditAttribution: aspilicious commentedLess fatals hopefully
Comment #18
aspilicious CreditAttribution: aspilicious commentedNeeded a reroll anyway... Lets see if this could work...
I have the feeling
isn't supported.
Tests fail when you assign the creation to a variable and save it at the same time..
Comment #19
yched CreditAttribution: yched commentedYup. Tedious in practice, but save() returns a flag indicating "created or updated".
So
$entity = entity_create(...)->save();
doesn't work.Comment #21
aspilicious CreditAttribution: aspilicious commentedAnother try... Refactored some more :s
Comment #23
aspilicious CreditAttribution: aspilicious commentedHmm
Comment #24
andypostA bit more clean-up, most of places with
field->id()
changed to$field_name
variable to make it easy to refactor latterAlso pushed to sandbox
PS: @aspilicious do you have access to sandbox?
Comment #26
pcambraRerroll of #24.
The only changes are due to the $field name
"$field_name[$langcode][0][value]" -> "{$field_name}[$langcode][0][value]"
And better type hinting:
Sorry for the lack of interdiff, hopefully this is green.
Comment #27
pcambraActually just realized that the interdiff should be applied to the sandbox so here it is for #26
Comment #28
swentel CreditAttribution: swentel commentedFantastic cleanup andypost and pcambra, thanks!
Comment #29
andypost@pcambra Merged to sandbox, thanx a lot
Comment #30
amateescu CreditAttribution: amateescu commentedRerolled after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.
Comment #31
alexpottCommitted 2de4615 and pushed to 8.x. Thanks!
Comment #32
fubhy CreditAttribution: fubhy commentedCan someone check whether all the places where we replaced field_delete_field() with $field->delete() are actually safe to do so? field_delete_field() has a if() check for whether the field actually exists. The places where we replaced it with the new syntax don't have that check. @see #2022769: Fix conversion of field_delete_field() in hook_uninstall()