Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Please refer the problem/motivation section of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Proposed resolution
Write EntityResourceTestBase subclass for the Item entity.
Remaining tasks
References
1. Follow-up of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
2. Subtask of #2824572: Write EntityResourceTestBase subclasses for every other entity type.
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff-2843752-26-30.txt | 642 bytes | shadcn |
#30 | entityresource_provide-2843752-30.patch | 13.09 KB | shadcn |
#26 | interdiff-2843752-24-26.txt | 1.24 KB | shadcn |
#26 | entityresource_provide-2843752-26.patch | 13.07 KB | shadcn |
#24 | interdiff-2843752-22-24.txt | 1.84 KB | shadcn |
Comments
Comment #3
shadcn CreditAttribution: shadcn at Chapter Three commentedWorking on this one instead since #2843755: EntityResource: Provide comprehensive test coverage for Message entity is currently blocked :)
Comment #4
Wim LeersWoot, thanks! :)
Comment #5
shadcn CreditAttribution: shadcn at Chapter Three commentedI'm attaching a WIP patch here.
It's failing because
\Drupal\aggregator\Entity\Item
has no canonical url but still returnstext/html; charset=UTF-8
. Hence the test below fails:Here's a related issue with some more info: #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL.
Wim, can you advise on how to fix the fails here? Thanks.
Note: I've marked
testPatch
andtestPatch
as skipped for now.Comment #7
Wim LeersGreat catch!
We're in
testPost()
, yet the code said this:Note the mention of
"GET"
!That does not make sense. I mirrorred this after
testGet()
. But of course that doesn't make sense. When you don't specify a format, thenhtml
is assumed by Drupal. Hence, you'll always get a HTML response.A similar problem exists a dozen lines of code further down.
I fixed both of those. Now you're getting this error:
I'll leave that to you again :)
Comment #9
shadcn CreditAttribution: shadcn at Chapter Three commentedThanks @Wim Leers. This is super helpful.
Comment #10
shadcn CreditAttribution: shadcn at Chapter Three commentedAdding another WIP patch here. This one fails (see the fail for testPost) because
\Drupal\aggregator\Entity\Item
has no uuid.Which is a good thing because now we can write the tests for #2835576: InvalidArgumentException: Field uuid is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField().
Comment #11
shadcn CreditAttribution: shadcn at Chapter Three commentedComment #12
shadcn CreditAttribution: shadcn at Chapter Three commentedComment #14
shadcn CreditAttribution: shadcn at Chapter Three commented@Wim Leers I believe we will have the same issues in #7 for
testPatch
andtestDelete
?Should we fix it in the related ticket here: #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL? I've updated the title of the ticket.
Comment #15
shadcn CreditAttribution: shadcn at Chapter Three commentedComment #16
Wim Leers#10 + #12:
\Drupal\aggregator\Entity\Item::baseFieldDefinitions()
:I just got that issue going again: #2149851-14: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field. If
Item
entities behaved as intended, then this would be a non-issue.Item
entities. So I'd rather not have that committed, and would prefer #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field to be fixed instead.#14 + #15:
Indeed.
I think so, yes. Posted a review there!
Comment #17
Wim Leers#2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL landed, this is now unblocked!
Comment #18
shadcn CreditAttribution: shadcn at Chapter Three commentedThanks @Wim Leers. Working on it now.
Comment #19
Wim LeersWoot, thanks!
Comment #20
shadcn CreditAttribution: shadcn at Chapter Three commentedOK let's try this one.
Note: I added a check for the uuid field and a @todo with a link to the issue. Let me know if this works.
Comment #22
shadcn CreditAttribution: shadcn at Chapter Three commentedForgot to include the EntityResourceTestBase fix. Let's try again.
Comment #23
Wim LeersSo close!
We should change this to:
That's more explicit.
s/array()/[]/
We can hardcode this ID to
1
AFAICT?Comment #24
shadcn CreditAttribution: shadcn at Chapter Three commentedUpdated. Thanks @Wim Leers :)
Comment #25
Wim LeersSorry… I noticed a few more small things:
Let's omit the
t()
call here.t()
is for interface translation. This is content.Why use
setPostedTime()
, but notsetFeedId()
,setTitle()
orsetLink()
?Let's use those setters instead of passing all the values in the
Item::create()
call. That's better/clearer.(It's also how e.g.
\Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::createEntity()
does it.)We can even hardcode this URL! :)
After this, I promise it's RTBC!
Comment #26
shadcn CreditAttribution: shadcn at Chapter Three commentedAll changes completed.
Comment #27
Wim LeersThanks!
Comment #29
Wim LeersAhhh, this only works on D8 installations that are installed in a domain's root, it will fail for subdir installations.
Use
base_path()
here. See e.g.\Drupal\Tests\rest\Functional\EntityResource\Comment\CommentResourceTestBase::getExpectedNormalizedEntity()
.Comment #30
shadcn CreditAttribution: shadcn at Chapter Three commentedUpdated.
Comment #31
Wim LeersComment #32
alexpottCommitted and pushed 8419d5e to 8.4.x and 769eb4b to 8.3.x. Thanks!
Comment #35
Wim Leers#2857086: EntityResourceTestBase: only test uuid for entity types that have uuid was closed as a duplicate of this :)