Added parameter types for some methods and fixed a wrong return type of the __get() method

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

edwardaa created an issue. See original summary.

edaa’s picture

Status: Active » Needs review
FileSize
1.77 KB
FeyP’s picture

Status: Needs review » Needs work

Thanks for the patch, @edwardaa. It applies and is definitely a documentation improvement. I noticed, that the @return tag for storageSettingsForm() is still missing a type hint. Let's add that to the patch while we're at it.

edaa’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Thanks for you feedback, added return type of array for storageSettingsForm()

FeyP’s picture

Assigned: edaa » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks, looks good. The patch still applies and it still is a documentation improvement. During the CI run I checked FieldItemBase and a few other random implementations to verify that the new type hints actually make sense. Marking as RTBC.

Wim Leers’s picture

Component: documentation » typed data system
Issue tags: +Documentation, +Needs subsystem maintainer review
+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -114,11 +114,11 @@ public function getFieldDefinition();
-   * @return \Drupal\Core\TypedData\TypedDataInterface
-   *   The property object.
+   * @return mixed
+   *   The property value.

@@ -128,9 +128,9 @@ public function __get($property_name);
-   * @param $property_name
+   * @param string $property_name
...
-   * @param $value
+   * @param mixed $value

Are you sure this is correct?

edaa’s picture

Yes, the __get() method returns the value of typed data rather than the typed data itself.

I am not pretty sure there is any case where the $property_name is anything else except 'string'

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Regarding #6, I had the same initial reaction when I saw the mixed. However, this is the default implementation provided by FieldItemBase:

public function __get($name) {
  // There is either a property object or a plain value - possibly for a
  // not-defined property. If we have a plain value, directly return it.
  if (isset($this->properties[$name])) {
    return $this->properties[$name]->getValue();
  }
  elseif (isset($this->values[$name])) {
    return $this->values[$name];
  }
}

So, that code comment would seem to support the change in the patch. And TypedData::getValue() does indeed return mixed (depending on the type) rather than a classed object.

  • xjm committed 87c8af2 on 8.5.x
    Issue #2887080 by edwardaa: Fix FieldItemInterface doc's parameter type...

  • xjm committed b665df5 on 8.4.x
    Issue #2887080 by edwardaa: Fix FieldItemInterface doc's parameter type...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

It's worth noting that while most coding standards should be cleaned up to fix the rule once across all of core, for scalar data types especially, the code has to be read carefully to ensure the documented types are correct and complete. (As #6 - #9 show.) This is even mentioned specifically at the end of https://www.drupal.org/core/scope#examples for that reason.

However, we should also check the implementations' and callers' typehints and fix them in the same issue scope, since those need to be checked anyway to verify the correct typehints are being added. Since these are all magic methods, there's no callers to check, but there are implementations on at least FieldItemBase. I reviewed FieldItemBase and confirmed that it does not add or override any documentation. So, the scope of this issue is sufficient and complete.

Committed and pushed to 8.5.x and 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.