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

CommentFileSizeAuthor
#12 revised-complexDataDefinition-2412363-12.patch734 bytesarla
#11 revised-complexDataDefinition-2412363-3.patch641 bytesmgifford
#3 revised-complexDataDefinition-2412363-3.patch641 byteszealfire

Comments

arla’s picture

arla commented
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

zealfire commented

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

zealfire commented
Status: Active » Needs review
StatusFileSize
new revised-complexDataDefinition-2412363-3.patch641 bytes

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

arla’s picture

arla commented
Status: Needs review » Reviewed & tested by the community

Looks good to me!

amateescu’s picture

amateescu commented
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

cmanalansan commented

Triaged as Novice.

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

fago’s picture

fago
Primary language German
Location Vienna
commented
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

xjm
she/her
Primary language English
commented
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

xjm
she/her
Primary language English
commented

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

arla commented

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

mgifford
he/him
Primary language English
commented
StatusFileSize
new revised-complexDataDefinition-2412363-3.patch641 bytes

Re-uploading patch for the bots.

arla’s picture

arla commented
StatusFileSize
new revised-complexDataDefinition-2412363-12.patch734 bytes

#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

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me.

catch’s picture

catch
he/him
Primary language English
commented
Title: ComplexDataDefinition::getPropertyDefinitions() needs to modify $this->propertyDefinitions » ComplexDataDefinition::getPropertyDefinition() does not need to check $this->propertyDefinitions
catch’s picture

catch
he/him
Primary language English
commented
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

berdir
Primary language German
Location Switzerland
commented
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.