This issue is very similar to #3150371: Vote/VoteInterface are documented improperly and incompletely only we're patching the VoteResult classes here. The patch is also going to be very similar because the VoteResult entity is almost identical to the Vote entity. Likewise for the interfaces. Please refer to the comments on that other issue first, if you have any questions about what is being done here.

VoteResultInterface exists to define the required public methods of vote result entities.

VoteResultInterface should be the place where these methods are defined and documented, because that is the purpose of the interface - to provide a public "contract" for the methods - their call parameters, their return types, and a description of what they are intended to do.

The whole notion of abstracting things to an interface is so alternative implementations may be provided and so those implementations will work as long as they are defined and perform according to the interface definition.

However, VoteResultInterface and VoteResult are coded improperly. Specifically:

  1. Most of the VoteResultInterface methods are documented with {@inheritdoc} This is wrong because VoteResultInterface does not inherit these methods from any other interface, so there is no documentation to "inherit". Likewise, VoteResultInterface is supposed to be DEFINING these methods, meaning this is the ONLY place they can be documented. Additionally, some of the documentation that is in VoteResultInterface is wrong - for example all the setters return $this but that is not documented, and there are some needed corrections. All of these {@inheritdoc} comments need to be replaced with real and accurate method documentation.
  2. VoteResultInterface extends EntityOwnerInterface, but it also REDEFINES all four of the the methods declared on EntityOwnerInterface. This is wrong. EntityOwnerInterface defines these methods and documents these methods, VoteResultInterface should NOT be redefining them - that defeats the whole purpose of extending EntityOwnerInterface and destroys the documentation provided by EntityOwnerInterface. These redeclarations need to be removed from VoteResultInterface.
  3. VoteResult::setCreatedTime() and VoteResult::getCreatedTime() are an essential part of the VoteResult entity definition, but they are not part of VoteResultInterface. That is wrong. If these methods are not defined on VoteResultInterface that means we have no way of providing alternative implementations of Vote because there is no way to enforce the required functionality of these implementations. The definition of get/setCreatedTime() needs to be added to VoteResultInterface.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR created an issue. See original summary.

TR’s picture

Status: Active » Needs review
FileSize
2.67 KB
Krzysztof Domański’s picture

Status: Needs review » Needs work

Looks good. One comment needs to be changed. Incorrect short description in doc comment of the getFunction() method.

   /**
-   * {@inheritdoc}
+   * Returns the vote value type.
+   *
+   * @return string
+   *   Name of the function to apply.
    */
   public function getFunction();
TR’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
742 bytes

Thanks for catching that. There was a lot of cut-and-paste and I missed changing that.

Krzysztof Domański’s picture

Status: Needs review » Reviewed & tested by the community
TR’s picture

Bump.

TR’s picture

Patch still applies and still passes tests, but there was an "unused use statement" so here's a new patch that removes that use statement.

pifagor’s picture

  • pifagor committed 31ceb9b on 8.x-3.x authored by TR
    Issue #3151918 by TR, Krzysztof Domański, pifagor: VoteResult/...
pifagor’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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