The getCreatedTime() method of the Contact is not inherited from any base-class or interface. The convention in other core modules is to declare the method in the interface the entity implements. In this case that would be ContactInterface

So I would like to suggest that ContactInterface declares the getCreatedTime() method.

CommentFileSizeAuthor
#16 content_entity_example-2504007-10191856-D8.patch1.82 KBfeyisayo
#10 content_entity_example-2504007-10019077.patch1.43 KBfeyisayo
#2 content_entity_example.patch905 bytesfeyisayo

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Component: Config Entity Example » Content Entity Example
Status: Active » Needs work
Issue tags: -content_entity_example

Sounds like a good example of best practices.

Do you have a patch?

feyisayo’s picture

feyisayo commented
Status: Needs work » Needs review
StatusFileSize
new content_entity_example.patch905 bytes

A patch. Of course. Sorry I didn't add this at the beginning. Here it is.

Status: Needs review » Needs work

The last submitted patch, 2: content_entity_example.patch, failed testing.

feyisayo’s picture

feyisayo commented

This test failure appears to be an issue with the test code itself.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Well the branch is green: https://qa.drupal.org/pifr/test/283843

If you click on the [view] link, you'll see that the test failed in the module that you changed. It's also a fatal error, and since we only changed the interface declaration here, it means there's a problem with that.

And the problem, it turns out, is that we're not declaring a method that's present in EntityChangedInterface: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

That method is EntityChangedInterface::getChangedTimeAcrossTranslations(), and can be implemented by using a trait that's neatly packaged inside D8: EntityChangedTrait.

So:

  • We have to remove getCreatedTime() from ContactInterface, because EntityChangedInterface already promises it.
  • Then we declare that ContactInterface extends EntityChangedInterface.
  • Then declare that Contact uses EntityChangedTrait.

Yay snap-together! :-)

And then add this information to the prose documentation part of Contact, in the class docblock. Explain that we're implementing EntityChangedTrait so that we can have timestamps.

And then run the tests locally to see what happens. :-)

feyisayo’s picture

feyisayo commented

Ah. I see. I have been working on the Beta 10 release of Drupal in which EntityChangedInterface only had the getChangedTime() method.

I should have updated to the latest version.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Yes, we're tracking the 8.0.x branch. So do that git pull origin 8.0.x and you're set.

feyisayo’s picture

feyisayo commented

Sorry, I have been away at an event. I'll redo the changes using your outline and resubmit the patch

feyisayo’s picture

feyisayo commented

Please disregard that comment. I didn't check the latest code before sending it.

feyisayo’s picture

feyisayo commented
StatusFileSize
new content_entity_example-2504007-10019077.patch1.43 KB

Hello Mile23,
Oh boy. I got this quite muddled up. I realised that sending the patch is still in order. I am sorry about the mix up.

Thanks for the clear instructions. You practically wrote the patch with words.

I have made the changes and run the tests locally. All tests passed.

Here it is

feyisayo’s picture

feyisayo commented
Status: Needs work » Needs review
pp’s picture

pp commented
Status: Needs review » Needs work

I reviewed the patch, it is looking good, but documentation is missing:

And then add this information to the prose documentation part of Contact, in the class docblock. Explain that we're implementing EntityChangedTrait so that we can have timestamps

feyisayo’s picture

feyisayo commented

Hello pp. Thanks for the review and I'm sorry for the delayed response.

Oh boy. How do I ask this? Documenting a patch.........could you point me to a link on how to do this?

pp’s picture

pp commented

Hi @feyisayo,

Your patch contains code. Documenting the code.

in the class docblock

Please read end of the comment of @Mile23 #2504007-5: ContactInterface should provide getCreatedTime() (this is what I quoted)

feyisayo’s picture

feyisayo commented

Thanks pp,
I have "duh" written on my face right now :)

I'll do that

feyisayo’s picture

feyisayo commented
StatusFileSize
new content_entity_example-2504007-10191856-D8.patch1.82 KB

Sorry this is coming quite late. I have been tied up with a number of things down here.

feyisayo’s picture

feyisayo commented
Status: Needs work » Needs review

  • Mile23 committed da42e83 on 8.x-1.x authored by feyisayo 
    Issue #2504007 by feyisayo, pp: ContactInterface should provide...
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Fixed

Thanks, @feyisayo and @pp!

Committed with minor coding standards changes.

feyisayo’s picture

feyisayo commented

Thanks for the direction @mile23 and for your help too @pp

Status: Fixed » Closed (fixed)

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