Problem/Motivation

ComplexDataDefinitionInterface documents getPropertyDefinitions() as "Gets an array of property definitions of contained properties." ComplexDataDefinitionBase however additionally expects the method to set the result to $this->propertyDefinitions. It leaves getPropertyDefinitions() abstract with {@inheritdoc}, so the method documentation is insufficient to anyone wanting to implement it.

Proposed resolution

Set $this->propertyDefinitions = $this->getPropertyDefinitions() in ComplexDataDefinitionBase::getPropertyDefinition(). Or maybe change the documentation to include something like "set the result to $this->propertyDefinitions".

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Arla’s picture

Issue summary: View changes
Issue tags: +Novice

As long as someone who is already well-informed in the typed data system can confirm this and decide on the appropriate resolution, it should be a Novice task to provide a patch.

zealfire’s picture

I would love to work on this and this is what i feel that since the function ComplexDataDefinitionBase::getPropertyDefinition() expects to set $propertyDefinitions when it does not exists so we should go we this: $this->propertyDefinitions = $this->getPropertyDefinitions().
If there is a yes then i may submit a patch.

zealfire’s picture

Status: Active » Needs review
FileSize
641 bytes

Please see mine comment:2 and submitting this patch to see if its passes test.Please review.
Thanks.

Arla’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

amateescu’s picture

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

Let's wait for a review from a typed data maintainer first :)

cmanalansan’s picture

Triaged as Novice.

Let's wait for a review from a typed data maintainer first :)

fago’s picture

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

Yep, that makes sense - we should not rely on the implementation internal.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Novice

So, from the summary:

ComplexDataDefinitionBase however additionally expects the method to set the result to $this->propertyDefinitions.

That's a dash of code smell for sure. Thanks for finding and patching this. And thanks @fago for the signoff, and @amateescu for making sure we got it. ;)

+++ b/core/lib/Drupal/Core/TypedData/ComplexDataDefinitionBase.php
@@ -29,7 +29,7 @@
     if (!isset($this->propertyDefinitions)) {
-      $this->getPropertyDefinitions();
+      $this->propertyDefinitions = $this->getPropertyDefinitions();
     }

I find it a little weird for the getter for one property to be setting a different property, and find myself wondering what happens if the results of the getter have changed since the property was set.

Wouldn't the following make more sense?

  public function getPropertyDefinition($name) {
    $definitions = $this->getPropertyDefinitions();
    if (isset($definitions[$name]) {
      return $definitions[$name];
    }
  }

Setting NR for feedback on that. (Also removing the novice tag since there are no novice tags at present.)

xjm’s picture

Forgot to mention -- if the goal is to statically cache the results of getPropertyDefinitions() to the property, then that method should be responsible for its own internal static cache and return early with the property. Having another method responsible for it seems a bit dubious. :)

Arla’s picture

I agree that it's weird that "singular" getter modifies the "plural" property. But I think the snipped suggested in #8 has the disadvantage that it pushes down the responsibility of static-caching the property to the implementor. How about implementing getPropertyDefinitions in the base class with caching, and declare abstract loadPropertyDefinitions?

  public function getPropertyDefinitions() {
    if (!isset($this->propertyDefinitions)) {
      $this->propertyDefinitions = $this->loadPropertyDefinitions();
    }
    return $this->propertyDefinitions;
  }

  abstract protected function loadPropertyDefinitions();

  public function getPropertyDefinition($name) {
    return isset($this->propertyDefinitions[$name]) ? $this->propertyDefinitions[$name] : NULL;
  }
mgifford’s picture

Re-uploading patch for the bots.

Arla’s picture

#3 duplicates the assignment to $this->propertyDefinitions, and #10 breaks BC. @xjm's #8 + #9 seems like the most reasonable approach.

But does it make sense to keep the declaration of $propertyDefinitions in Base? Pushing it down will break BC.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me.

catch’s picture

Title: ComplexDataDefinition::getPropertyDefinitions() needs to modify $this->propertyDefinitions » ComplexDataDefinition::getPropertyDefinition() does not need to check $this->propertyDefinitions
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed f7a24a4 on
    Issue #2412363 by Arla, zealfire, mgifford, xjm, fago:...

  • catch committed b977a2c on
    Issue #2412363 by Arla, zealfire, mgifford, xjm, fago:...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: revised-complexDataDefinition-2412363-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Fixed

Uh, weird status change. Setting back to fixed.

Status: Fixed » Closed (fixed)

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