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.
This is a follow-up for #2824572-23: Write EntityResourceTestBase subclasses for every other entity type.:
@Wim Leers:
Once we have test coverage for all content entity types, we should add a test that checks that all core content entity types have functional REST test coverage. That way, we ensure that no new content entity types can be added that do not have functional REST test coverage!
Same for config entity types, of course.
Comment | File | Size | Author |
---|---|---|---|
#58 | xjmepic.png | 1.01 MB | Anonymous (not verified) |
#55 | 2868035-55.patch | 5.69 KB | Wim Leers |
#48 | 2868035-48.patch | 5.54 KB | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedvaplas created an issue. See original summary.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedHah!
basename
- works only for Windows (because use '/' instead of '\'). Replaced byexplode
.'sun'
to prevent the cutting of spaces at the beginning of the message.Comment #11
Wim Leers@vaplas: thanks so much for creating this issue! And then on top of that, you also created a patch! Awesome :)
I think this looks ready. We now need to get it to green… which means that we need to fix every issue mentioned in #2824572: Write EntityResourceTestBase subclasses for every other entity type. :) So postponing it on that!
HAHAHAHHAHAHAHAHAHAHAHHAHAHAHAHAHAHAHAHAHAHAH
THANK YOU :D
Comment #13
Wim LeersNow that #2824572: Write EntityResourceTestBase subclasses for every other entity type. is finally reaching the end, let's drive this patch to completion.
Comment #14
Wim Leerscontent_moderation_rest_resource_alter()
ensures theContentModerationState
entity type is never exposed via REST. We need to take removed REST resource plugin definitions into account.Comment #16
Wim LeersAdditionally, REST test coverage for entity types being added by experimental modules don't live in
\Drupal\Tests\rest
, but inDrupal\Tests\experimental_module_name\Functional\Rest
.This updates the test coverage to look in multiple places for the test coverage.
Comment #17
Wim Leers#13 is the same as #9, just with added comments. But there's >5 months of progress with REST test coverage going from #9's test result to #13's.
#9:
#13:
Comment #20
kristiaanvandeneyndeAll hail the ASCII llama. We need more test messages like that! Also I like the idea of test coverage covering test coverage.
Comment #21
Wim Leers#17 showed that we are at 34/42. #18's correction makes this 34/41 (
ContentModerationState
is not exposed via REST). #19's correction makes this 35/41 (theWorkflow
entity type's test coverage lives in theworkflows
module, not in therest
module).So we're down to 6:
But that lists
FieldLayoutEntityViewDisplay
rather thanEntityViewDisplay
andFieldLayoutEntityFormDisplay
rather thanEntityFormDisplay
. This is because the experimentalfield_layout
module does this:Therefore the 6 remaining issues in #2824572: Write EntityResourceTestBase subclasses for every other entity type. do in fact match! (This patch adds the entity type ID to make the output a bit clearer.)
Comment #22
Wim Leers:P
I'm not sure core committers will be willing to commit it like this… although I agree that'd be very cool.
Not my idea! The Help module pioneered this, to ensure that every module has a
hook_help()
implementation :)Comment #23
kristiaanvandeneyndeWe need more funny stuff like this to be honest :(
Cool, you learn something every day.
Comment #25
Wim LeersGreat, now the failed test output is crystal clear:
Comment #26
Wim LeersLet's show the actual number of blockers.
Comment #27
Wim LeersThis blocks #2910883: Move all entity type REST tests to the providing modules.
Comment #28
Wim LeersOkay, back to postponed until #2824572: Write EntityResourceTestBase subclasses for every other entity type. is done!
Comment #29
dawehnerLet's give
$all_modules
a differnet name as its no longer all modules :)Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedSuper correction here! Thank you, @Wim Leers!
And many thanks to @kristiaanvandeneynde for supporting this "llama message", it is very inspiring! But of course, this message is not a fundamental moment here) Just a little fun for the rest folks. After a rich history with this test coverage, why not?)
In any case, we will need to update the message, after all the entities are completed. Perhaps more emphasis on help info for new entities, than on progress.
#29: sure, thank you @dawehner :) I will update this in the future experiments.
Comment #31
Wim Leers#2843780: EntityResource: Provide comprehensive test coverage for EntityFormMode entity landed!
Comment #32
Wim LeersOnce #2800873: Add XML GET REST test coverage, work around XML encoder quirks lands, we'll need to update this to verify there's XML test coverage too!
Comment #33
Wim Leers#2843781: EntityResource: Provide comprehensive test coverage for EntityViewMode entity landed!
Comment #34
Wim Leers#2843755: EntityResource: Provide comprehensive test coverage for Message entity landed!
Comment #35
Wim Leers#2843764: EntityResource: Provide comprehensive test coverage for EntityFormDisplay entity and #2843765: EntityResource: Provide comprehensive test coverage for EntityViewDisplay entity landed!
Comment #36
Wim LeersLet's get this patch tested again. We should be down to one failure now.
Also addressing @dawehner's feedback in #29 and my own nits:
We should also exclude experimental modules — it's okay for experimental modules to not yet have full REST test coverage.
These should all use
$this->container->get(…)
Extraneous newline.
We should fix the language here.
Comment #37
Wim LeersAssuming this comes back with the expected failure for the sole remaining REST test coverage issue (#2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler), I think we should add a work-around for that for now so that this can already be committed. Then at least we know we won't be adding more entity types without REST test coverage: this patch will ensure we'll never regress!
Which means this issue would no longer be blocked! (Updating issue title accordingly.)
Thoughts?
Comment #38
tedbow@Wim Leers I think the work around is good idea, the file issue could take a while and in the meantime we won't have test to force REST coverage for any new entities.
All my test messages are now llame in comparison.
Comment #40
Wim Leers#38: Great! Does that mean this becomes RTBC? :)
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat a beautiful play of words! 💎💎💎
Maybe also add something like this
For prevent fix #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler without performing this @todo?
Edit: elseif of course
Comment #42
dawehnerThis is weird, why do we do that. This makes it harder to read IMHO
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedThis function does not affect the test. It's just a small fan, where more convenient to work with variables, the length of which coincides with their values.
Example:
vs
Although this code
is no longer relevant, because both numbers are two-valued. Good catch!
#41 + #42 done.
Comment #44
dawehner🔧 You could get rid of the if by negating the entire statement and return it.
Now that we just assign a variable I see even less of a reason to assign them to some new variable locally.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commented#44: thanks again! Done.
Comment #47
Wim Leers$module->status === 0
, which is why the=== FALSE
check didn't work, but== FALSE
does work.Comment #48
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat insidious status value. Thank you, @Wim Leers, that clarified it! Then how about this.
Comment #49
Wim Leers👍
Comment #50
Wim Leers90% of the work here was done by @vaplas. This has been ready for about a week now, with no one else stepping up to RTBC it. So I will.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedRandom fail #2906317: Random fail due to problems with database
Comment #53
borisson_This has been RTBC for a week, but I figured another review would help the committers be more certain that this works as expected.
This is awesome, it will automatically alert us when the rest coverage for files is fixed!
RTBC++
Comment #54
larowlanthe english is off here
nice work though
Comment #55
Wim LeersFixed.
Comment #57
xjmThis is epic!
I found a few more docs nitpicks, but I was so excited about this patch that I fixed them on commit:
...That almost went badly because I struggle to type "XML" correctly since it's so close to my username.
Committed and pushed to 8.5.x!! 🎆🎆🎆
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commented#53, #54: thanks for additional review and positive feedback!
#55: thanks as always!
#57: ❤️
Comment #59
xjmThat is the most amazing thing I have seen on the entire internet.
Comment #60
Wim Leers#57: 🎉 😁
#58: 😵 🤣
WHERE DO YOU EVEN FIND THOSE THINGS VAPLAS???!?!?!?!?!?!??!!?!??!!!!!!
Comment #61
Wim Leers@vaplas already updated #2843139, to remove the
@todo
that this introduced: #2843139-96: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler.And I just updated #2800873 to ensure that the "test coverage test" that this issue introduced also verifies that there is XML REST test coverage for every entity type: #2800873-163: Add XML GET REST test coverage, work around XML encoder quirks.