Clean up normalizer parameter and variable names to use "field_items" consistently etc

Looks like the code in FieldItemNormalizer::createTranslatedInstance() already got changed?

Looks like the code in FieldItemNormalizer::createTranslatedInstance() already got changed?

Indeed.

Rerolled #4.

This 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.

No, 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

Yep. 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 :)

ok, awesome!

I 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.

I made a new patch to cover on the most recent 8.4.x

Adding Needs Review Status for Queuing

Corrected the patch for 8.4.x

Thank 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.

Fatal error: Call to undefined method Drupal\Core\Field\BaseFieldDefinition::getFieldName() in /var/www/html/core/modules/hal/src/Normalizer/FieldItemNormalizer.php on line 41

It looks like the method is getName and not getFieldName.

Redo 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.

Patch with getName() method corrected.
Interdiff attached.

Sorry, wrong interdiff file.

The 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?

Re-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.

Small changes like this sum up over time!

+++ b/core/modules/hal/src/Normalizer/FieldNormalizer.php
@@ -62,14 +61,14 @@ public function normalize($field, $format = NULL, array $context = []) {
    */
-  protected function normalizeFieldItems($field, $format, $context) {
-    $normalized_field_items = [];
-    if (!$field->isEmpty()) {
-      foreach ($field as $field_item) {
-        $normalized_field_items[] = $this->serializer->normalize($field_item, $format, $context);
+  protected function normalizeFieldItems($field_items, $format, $context) {
+    $normalized_items = [];
+    if (!$field_items->isEmpty()) {
+      foreach ($field_items as $field_item) {
+        $normalized_items[] = $this->serializer->normalize($field_item, $format, $context);
       }
     }
-    return $normalized_field_items;
+    return $normalized_items;
   }
 
 }

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.

I agree with #36. The last hunk of changes seems unnecessary. The rest looks good!

added patch

Thank you @harsha012!

Thanks!

Agreed, +1!

+++ b/core/modules/hal/src/Normalizer/FieldNormalizer.php
@@ -38,15 +38,14 @@ public function normalize($field, $format = NULL, array $context = []) {
-    $normalized = NestedArray::mergeDeepArray($normalized_field_items);
-    return $normalized;
+    return NestedArray::mergeDeepArray($normalized_field_items);

Technically 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:

    // If the fields to use were specified, only output those field values.     
    if (isset($context['included_fields'])) {
      $fields = [];
      foreach ($context['included_fields'] as $field_name) {
        $fields[] = $entity->get($field_name);
      }
    }
    else {
      $fields = $entity->getFields();
    }
    foreach ($fields as $field) {
      // Continue if the current user does not have access to view this field.  
      if (!$field->access('view', $context['account'])) {
        continue;
      }

      $normalized_property = $this->serializer->normalize($field, $format, $context);

(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.

@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

#45: nice spot, although that's technically expanding the issue scope ;) Updating issue title to reflect #45+#46.

+++ b/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php
@@ -21,9 +21,9 @@ public function normalize($object, $format = NULL, array $context = []) {
-    foreach ($object as $name => $field) {
...
+    foreach ($object as $name => $field_items) {

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.)

Good point!

Thank you @Wim Leers!

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!