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.
Subtask of: #2824572: Write EntityResourceTestBase subclasses for every other entity type.
Write EntityResourceTestBase subclass for the MenuLinkContent entity.
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff.txt | 1.32 KB | Anonymous (not verified) |
#40 | 2832859-40.patch | 13.38 KB | Anonymous (not verified) |
#36 | interdiff-words-17-34.txt | 5.69 KB | Wim Leers |
#36 | interdiff-17-34.txt | 7.67 KB | Wim Leers |
#35 | iterdiff-33-34.txt | 3.86 KB | Anonymous (not verified) |
Comments
Comment #2
harings_rob CreditAttribution: harings_rob commentedComment #3
harings_rob CreditAttribution: harings_rob commentedComment #4
harings_rob CreditAttribution: harings_rob commentedHi,
Attached I have added the patch. But I am running into a knowledge issue here.
The test seems pretty straight forward, so I guess that I either:
- Completely misunderstand the logic,
- Menu link content permissions don't make sense/Menu link content should be a config entity
CheckAccess seems to always require 'administer menu': \Drupal\menu_link_content\MenuLinkContentAccessControlHandler::checkAccess
Some hints/guidance would be welcome here.
Comment #5
Wim LeersWoot, thanks Rob!
To answer your questions regarding access handling: you're right that it's strange right now. We have similar strangeness for vocabulary & term entities, we have #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission + #2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission to fix that. You could open a similar issue for
menu_link_content
entities (but please don't do it in this issue, that would be out of scope). I think it'd make sense to add a more granular permission for viewing Menu Link content entities.The other
EntityResourceTest
subclasses also don't have these comments, so let's not start doing so here.All HTTP methods require the same permission for now, so we can simplify this.
Comment #6
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #8
Wim LeersWoah, @webflo is in the house! I didn't know you were working on REST stuff :)
Comment #10
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedI work on it during SprintWeekend.
Comment #11
sumanthkumarc CreditAttribution: sumanthkumarc at Azri Solutions commented@webflo, i'm also working on similar issue, https://www.drupal.org/node/2843750 and following you on this. Any directions for me??
Comment #12
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedSome progress on hal+json.
Comment #13
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #15
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #17
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #18
Wim LeersNice improvements to the base test class! :)
This looks perfect, thanks! :)
Comment #20
alexpottCommitted ef0a872 and pushed to 8.4.x. Thanks!
Committed 48ec098 and pushed to 8.3.x. Thanks!
Coding standards fixed on commit.
Comment #25
xjmUnfortunately, this seems to be failing in HEAD, so I reverted it. I'll add retests on the patch.
E.g.: https://www.drupal.org/pift-ci-job/596766
Comment #26
Wim LeersAh yes, the failing tests make sense: this needs a reroll after #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out.
Apparently the RTBC queue is so long that testbot didn't actually do that yet, despite it landing about 2 days before this got committed.
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedPatch applies successfully on both 8.3.x and 8.4.x so does not appear to be a re-roll, but still Needs Work to address test failures.
Comment #28
harings_rob CreditAttribution: harings_rob commentedAre the failures happening due to normalization?
Wanted to take a look, but not sure where to start
This seems like an actual bug in the implementation (Meaning test is ok). Needs a new issue?
This looks to me a formatting issue, might be due to my environment.. The same happens on Post and Delete..
Can we trigger testbot again?
Comment #29
Wim Leers#28: no that means this entity type has unauthorized access error messages that deviate from the default ones. That's fine.
It means we just need to override
getExpectedUnauthorizedAccessMessage()
. For an example, see\Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::getExpectedUnauthorizedAccessMessage()
.Comment #30
harings_rob CreditAttribution: harings_rob commentedOk so what about the other one?
Comment #31
Wim LeersAll three errors are about that AFAICT :)
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commented#20 coding standards + #29
getExpectedUnauthorizedAccessMessage
+ cheat.About cheat:
It seems
neutral()
still don't tell anything. But maybe we need bit more refactoring this part of code. Example::)
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedone of fix not included :(
Comment #34
shadcn CreditAttribution: shadcn at Chapter Three commentedNit: indentation. Thanks :)
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedlet's fix it + few commas.
Comment #36
Wim LeersI compared #17 and #35 by applying both locally, and diffing them. Because it also fixes an indentation issue, I also had to use
--word-diff
.This looks perfect.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commented@Wim Leers, thank you for review! Come back to RTBC.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentednit reroll after #2751325: All serialized values are strings, should be integers/booleans when appropriate
Comment #43
Wim LeersThanks!
Comment #44
Wim LeersAs of #2577923-106: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property, this is a blocker for #2577923.
Comment #45
alexpottCommitted and pushed e4461e6 to 8.4.x and 0dcbf65 to 8.3.x. Thanks!
I've backported this to 8.3.x as it 99.9% additional test coverage and the remainer just helps with REST adoption and has no BC concerns.
This is the one non test change in the patch and it's helpful and totally non-controversial.
Comment #48
Wim LeersAwesome :) Thanks, @alexpott, for your thoughtfulness!
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedWoot!)
+ return AccessResult::neutral("The 'administer menu' permission is required.")->cachePerPermissions();
This change has test coverage too. Without it we have fail:
But if we adding code:
we have other fail:
So change
MenuLinkContentAccessControlHandler::checkAccess()
did not come for fun)EntityResourceTestBase
is a very cool, because it makes necessary to correct such places :)Comment #50
Wim Leers@vaplas: thanks for explaining the reason for that change so clearly! I'm sure it'll be helpful for future readers.
I'm glad you feel that way :)