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.
Problem/Motivation
Core has settled on using "field items" for variables that contain a FieldItemListInterface
object.
Proposed resolution
Also do this for normalizers.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#48 | interdiff-2143089.txt | 952 bytes | dawehner |
#48 | 2143089-48.patch | 6.03 KB | dawehner |
#46 | interdiff-2143089.txt | 2.85 KB | dawehner |
#46 | 2143089-46.patch | 5.82 KB | dawehner |
#40 | interdiff-35-40.txt | 815 bytes | harsha012 |
Comments
Comment #1
yched CreditAttribution: yched commentedFound a couple more in other normalizer classes.
As a side note, the code in FieldItemNormalizer::createTranslatedInstance() looks really weird ? (left untouched)
Comment #3
yched CreditAttribution: yched commentedSilly me. $item->getName() is the delta, not the field name.
Comment #4
yched CreditAttribution: yched commentedExtending the scope a bit, simplifies some really weird / needlessly convoluted code in FieldItemNormalizer::createTranslatedInstance() (used for denormalize())
The logic of what's done here escapes me, and it looks like it would use some double-checking given the current state of the entity/field translation API. The patch leaves the logic untouched, just simplifies it.
Comment #6
damiankloip CreditAttribution: damiankloip commentedLooks like the code in FieldItemNormalizer::createTranslatedInstance() already got changed?
Comment #8
Wim LeersComment #9
Wim LeersIndeed.
Rerolled #4.
Comment #14
Wim LeersComment #15
mradcliffeThis patch is most likely needing to be re-rolled, and we probably should go ahead and add @internal to the hal field item and field normalizer classes because of method signature changes.
Comment #16
damiankloip CreditAttribution: damiankloip at Acquia commentedNo, I don't think these classes should be internal, most classes implementing some custom behaviour for fields would likely extend these. I'm not sure PHP cares about actual parameter names, more the types? So I don't think existing code would actually break? https://3v4l.org/5tCCK
Comment #17
Wim LeersYep. But at DrupalCon, there's been talk to make https://www.drupal.org/core/d8-bc-policy explicit, which apparently might also mean marking every single
protected
method as@internal
(which I personally think is going too far). @mradcliffe was considering doing that as part of this issue. We talked at DrupalCon, and I explained to him that's polluting the issue scope; that that is something that should happen in bulk in another issue. He agreed. So we're good :)Comment #18
damiankloip CreditAttribution: damiankloip at Acquia commentedok, awesome!
Comment #19
cjgratacos CreditAttribution: cjgratacos as a volunteer commentedI was thinking of picking this issue, and do the reroll and cleanup, but I am a bit confused with the description of the issue with the changes inside the patch. The issue is stating that we need to change "$fields" with "$items" on the function signature uses a FieldItemList object, yet what was done is append the "_item" keyword to the "$field" variable. Was this the intend of the issue, or to just completely replace "fields" or "$field" with "$items" or "$item". Thanks in advance.
Comment #20
imadalin CreditAttribution: imadalin as a volunteer and at Numiko commentedI made a new patch to cover on the most recent 8.4.x
Comment #21
pritish.kumar CreditAttribution: pritish.kumar at OpenSense Labs commentedAdding Needs Review Status for Queuing
Comment #23
c.nish2k3 CreditAttribution: c.nish2k3 as a volunteer commentedCorrected the patch for 8.4.x
Comment #25
mradcliffeThank you for getting a re-rolled patch, @imadalin, @c.nish2k3. It would help if in #23 you could create and upload an interdiff between patches so that it is easier to review the changes between patches.
It looks like there are some test failures.
It looks like the method is getName and not
getFieldName
.Comment #26
imadalin CreditAttribution: imadalin as a volunteer and at Numiko commentedRedo patch.
PS. Anybody would have an idea why a patch made from PHPStorm would fail? I use that on other projects and never had an issue.
Comment #27
imadalin CreditAttribution: imadalin as a volunteer and at Numiko commentedPatch with getName() method corrected.
Interdiff attached.
Comment #28
imadalin CreditAttribution: imadalin as a volunteer and at Numiko commentedSorry, wrong interdiff file.
Comment #29
imadalin CreditAttribution: imadalin as a volunteer and at Numiko commentedComment #32
imadalin CreditAttribution: imadalin as a volunteer and at Numiko commentedComment #33
John Cook CreditAttribution: John Cook at Creode commentedThe code look good, but I'm a little confused.
The issue summary says that we should be using the variable name "$items" but the patch appears to using "$field_items". Am I understanding the issue correctly?
Comment #34
vegantriathleteComment #35
bradjones1Re-rolled for 8.4.x; changes in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX mean, I think, that the changes to
FieldItemNormalizer.php
do not need to be applied here, akin to the change observed in #9 above.Comment #36
dawehnerSmall changes like this sum up over time!
I don't get this rename to be honest. The field name afterwards seems worse than before ... At least for me
$normalized_field_items
was really descriptive.Comment #37
vegantriathleteComment #39
Wim LeersI agree with #36. The last hunk of changes seems unnecessary. The rest looks good!
Comment #40
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedadded patch
Comment #41
dawehnerThank you @harsha012!
Comment #42
Wim LeersThanks!
Comment #43
damiankloip CreditAttribution: damiankloip at Acquia commentedAgreed, +1!
Comment #45
xjmTechnically this is out of scope. Not too concerned about that. However...
... I am pretty sure at least
ContentEntityNormalizer
needs the same change. In this bit:(I could be wrong; please confirm.)
The parent class (
ListNormalizer
) has it named as$fieldItems
rather than$field_items
. Both are allowable in our coding standards currently.Comment #46
dawehner@xjm
Nice spot!
I went through all normalizers and found an additional instance where we named something field, even its a typed data property.
Personally meh about field_items vs. fieldItems
Comment #47
Wim Leers#45: nice spot, although that's technically expanding the issue scope ;) Updating issue title to reflect #45+#46.
If we're making this change, then we should probably also rename
$object
to$entity
.(Which is also what
\Drupal\hal\Normalizer\ContentEntityNormalizer::normalize()
already does.)Comment #48
dawehnerGood point!
Comment #49
Wim LeersComment #50
dawehnerThank you @Wim Leers!
Comment #54
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!