Problem/Motivation

Coding is easier if classes have interfaces. There's a pattern of interfaces especially for entity classes. The ShareMessage module introduces a ShareMessage entity.

Proposed resolution

Create ShareMessageInterface in src/ and define the public ShareMessage-specific methods (including doc-comments) there. The ShareMessage class should only have {@inheritdoc} and the actual method implementations.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 share-message-interface-2577653-11.patch11.89 KBtduong
#11 interdiff-2577653-10-11.txt4.66 KBtduong
#10 share-message-interface-2577653-10.patch6.83 KBtduong
#10 interdiff-2577653-5-10.txt3.84 KBtduong
#7 share-message-interface-2577653-7.patch6.45 KBtduong
#7 interdiff-2577653-5-7.txt5.92 KBtduong
#5 share-message-interface-2577653-5.patch4.23 KBsdstyles
#3 share-message-interface-2577653-3.patch4.23 KBsdstyles

Comments

Arla created an issue. See original summary.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Assigned: Unassigned » joshi.rohit100
sdstyles’s picture

sdstyles commented
Status: Active » Needs review
StatusFileSize
new share-message-interface-2577653-3.patch4.23 KB
joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Assigned: joshi.rohit100 » Unassigned
Status: Needs review » Needs work 
+++ b/src/ShareMessageInterface.php
@@ -0,0 +1,80 @@
+  /**
+   * Set sharemessage status.
+   */
+  public function setStatus($label);
+

should be $status

sdstyles’s picture

sdstyles commented
Status: Needs work » Needs review
StatusFileSize
new share-message-interface-2577653-5.patch4.23 KB
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Yeah, looks pretty fine.

+++ b/src/Entity/ShareMessage.php
@@ -8,6 +8,7 @@ namespace Drupal\sharemessage\Entity;
+use Drupal\sharemessage\ShareMessageInterface;

I think the whole purpose of this interface is that it is then used as a replacement of the ShareMessage entity where it makes sense. I guess there are a few locations.

What confuses me is, the interface declares 8 methods, but i only see 4 inheritdoc. It seems some more methods need an update.

tduong’s picture

tduong commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2577653-5-7.txt5.92 KB
new share-message-interface-2577653-7.patch6.45 KB

It seems like there was already 4 inherits (setLabel, setTitle, getStatus, setStatus) and the other 4 methods still had their doc-comments, so I guess that @sdstyles has taken all the public methods and defined them in the Interface.

Added some new methods added after @sdstyles's patch and their documentations.

I'll check for the usage of these methods asap.

Status: Needs review » Needs work

The last submitted patch, 7: share-message-interface-2577653-7.patch, failed testing.

arla’s picture

arla commented

The patch does not apply, seems like it's wrongly generated. Also the interdiff looks odd?

  1. +++ b/src/ShareMessageInterface.php
@@ -0,0 +1,134 @@
+   *    A human-readable label representing the internal Share Message label.

    One space too much after the * asterisk. Same for many other @param and @return lines. 

  2. +++ b/src/ShareMessageInterface.php
@@ -0,0 +1,134 @@
+  /**
+   * Returns Open Graph meta tags for <head>.
+   */
+  public function buildOGTags($context);

    Missing @param and @return.

tduong’s picture

tduong commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2577653-5-10.txt3.84 KB
new share-message-interface-2577653-10.patch6.83 KB

Ok, trying again.

Fixed these smelly doc-codes. :)

tduong’s picture

tduong commented
StatusFileSize
new interdiff-2577653-10-11.txt4.66 KB
new share-message-interface-2577653-11.patch11.89 KB

Replaced ShareMessage with ShareMessageInterface in the files that use ShareMessage entity.

arla’s picture

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

Looks good to me now!

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Thank you, nice cleanup, committed! :-)

Status: Fixed » Closed (fixed)

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