VoteInterface exists to define the required public methods of vote entities.

VoteInterface 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, VoteInterface and Vote are coded improperly. Specifically:

  1. Most of the VoteInterface methods are documented with {@inheritdoc} This is wrong because VoteInterface does not inherit these methods from any other interface, so there is no documentation to "inherit". Likewise, VoteInterface 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 VoteInterface 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. VoteInterface 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, VoteInterface 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 VoteInterface.
  3. Vote::setCreatedTime() and Vote::getCreatedTime() are an essential part of the Vote entity definition, but they are not part of VoteInterface. That is wrong. If these methods are not defined on VoteInterface 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 VoteInterface.
  4. The Vote entity has documentation for the methods postSave() and postDelete() which are defined by the ContentEntityBase. This is wrong. We should be using {@inheritdoc} HERE in the Vote entity since the Vote entity DOES inherit this documentation from the ContentEntityBase.

I'm marking this as "Major" because there are some fundamental problems with the Vote entity implementation that I would like to clean up, but that can't be done until we first sort out this division of responsibility between the interface and the implementation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TR created an issue. See original summary.

TR’s picture

TR’s picture

Issue summary: View changes
TR’s picture

Patch in #2 only changed VoteInterface. I forgot to include the changes to Vote. Here is a new patch.

TR’s picture

Issue summary: View changes
TR’s picture

And one more patch with a coding standards fix - now that the redefinition of the EntityOwnerInterface methods is removed from VoteInterface we no longer need one of the 'use' statements.

Krzysztof Domański’s picture

Status: Needs review » Needs work

1. One more issue. IDE warning in src/Entity/Vote.php "Return value is expected to be 'array', 'int' returned."

 * @return array
 *   An array of default values.
 */
public static function getCurrentUserId() {
  return \Drupal::currentUser()->id();
}

2. The potential risk of breaking a Backward Compatibility. Child classes may not have this method defined.

The definition of get/setCreatedTime() needs to be added to VoteInterface

Contrib modules do not extend this interface. See http://grep.xnddx.ru/search?text=VoteInterface&filename=

What about custom modules?

3. Additional question (out-of-scope)

The Vote entity has documentation for the methods postSave() and postDelete() which are defined by the ContentEntityBase. This is wrong. We should be using {@inheritdoc}

Do we need to call the parent method here parent::postSave()? See #2898847: Vote list cache tag not invalidated when saving a vote

--- a/src/Entity/Vote.php
+++ b/src/Entity/Vote.php
@@ -252,6 +252,8 @@ class Vote extends ContentEntityBase implements VoteInterface {
    * @param bool|true $update
    */
   public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+    parent::postSave($storage, $update);
pavnish’s picture

Assigned: Unassigned » pavnish

Working on it

pavnish’s picture

Assigned: pavnish » Unassigned
Status: Needs work » Needs review
FileSize
5.2 KB
1.25 KB

@Krzysztof Domański I have update the patch plz review

RE #7.1 There was wrong comment now the comments has been changed.
RE #7.2 The definition of get/setCreatedTime() already exist in VoteInterface.
RE #7.3 Yes you are absolutely correct.
RE #7.4 Yes we might need to be add parent::postSave() .

TR’s picture

#7.1 Yes, we can do that, and same with the return value for getCurrentIp(). (*See below)

#7.2 I don't understand the comment. The implementation of the get/setCreatedTime() methods are still there in Vote after this patch, but now they are also defined on the VoteInterface. If contrib/custom extends Vote then they will still have these methods. If they implement VoteInterface instead, then they don't/can't currently work without those methods defined - they would already be broken if they don't have those methods. So I don't see how there could be a backwards compatibility problem.

#7.3 Noted that this was out-of-scope. The changes to postSave() and postDelete() ARE needed but should be done in a separate issue #2898847: Vote list cache tag not invalidated when saving a vote is already open for that. #9 doesn't do this correctly anyway, so rather than going back and forth trying to fix it here's a new patch with just #7.1 changed.

(* As part of the "some fundamental problems with the Vote entity implementation that I would like to clean up", getCurrentUserId() should be deleted entirely and replaced by EntityOwnerTrait- but that's WAY out-of-scope of this issue and I will post a new patch for that after we're done here.)

Krzysztof Domański’s picture

Status: Needs review » Reviewed & tested by the community

#7.2 Right, I didn't notice it before.

The implementation of the get/setCreatedTime() methods are still there in Vote after this patch, but now they are also defined on the VoteInterface.

pifagor’s picture

  • pifagor committed ba82e76 on 8.x-3.x authored by TR
    Issue #3150371 by TR, pavnish, Krzysztof Domański: Vote/VoteInterface...
pifagor’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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