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 Menu 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 |
---|---|---|---|
#14 | rest_menu-2843783-14.patch | 14.74 KB | Wim Leers |
#8 | interdiff-5-8.txt | 2.2 KB | Anonymous (not verified) |
#8 | rest_menu-2843783-8.patch | 7.93 KB | Anonymous (not verified) |
#5 | interdiff-3-5.txt | 2.06 KB | Anonymous (not verified) |
#5 | rest_menu-2843783-5.patch | 8.51 KB | Anonymous (not verified) |
Comments
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch will fail. Way to pass:
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #6
Wim LeersLooks perfect, thanks!
Comment #7
alexpottThis needs more discussion. As per the other issues. See #2843772-15: EntityResource: Provide comprehensive test coverage for DateFormat entity
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #9
alexpott@vaplas - we still need to discuss what is the right generic approach to viewing config entities on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commented@alexpott, thank you for the additional clarification and the special issue. I just wanted to connect Mr. Bot to the discussion too :).
Postponed by #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST.
Comment #11
Wim LeersYep, this is blocked on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST reaching consensus.
Comment #12
Wim LeersConsensus was achieved! Quoting #2870018-35: Should the 'access content' permission be used to control access to viewing configuration entities via REST:
Turns out #8 already changed it in the necessary way! :D
Comment #13
clemens.tolboomI do not see the mentioned #12
This issue is on 8.3.x. Is that correct?
Comment #14
Wim LeersChanging access control handlers means this can only be 8.4.x, good catch.
You don't see the "if has VIEW or ADMIN permission", because that'd require adding new permissions, which has yet other implications. That was the principle, but in applying the principle, we cannot add permissions in these issues; adding VIEW permissions must happen in other issues.
And per #2870018-47: Should the 'access content' permission be used to control access to viewing configuration entities via REST as well as #2843772-23: EntityResource: Provide comprehensive test coverage for DateFormat entity and subsequent comments, we must convert the
view
operation to theview label
operation in the access control handler, to not break BC. Full test coverage added.Comment #15
tedbowLooking at this access controller and #2843772: EntityResource: Provide comprehensive test coverage for DateFormat entity aren't a bunch of config entity access controllers going to be exactly the same?
Would it make sense to make
Then we could set all config entities that follow this pattern to use ConfigEntityAccessController directly?
If we can't do ConfigEntityAccessController could either have
ConfigEntityAccessHandlerTestBase extends KernelTestBase
or
ConfigEntityAccessHandlerTrait
Looking at #2843772: EntityResource: Provide comprehensive test coverage for DateFormat entity all these tests are going to be very similar. Just a thought.
If we don't want to do any of that refactoring I think this good to RTBC!
Comment #16
Wim LeersThat's … a very good point :) Although I have to add that
isLocked()
is not actually on a generic interface yet. I think extracting aLockableInterface
is going to be a hard requirement. I count at least 5 config entity types that'd need to use it (DateFormat
,NodeType
,Menu
,ConfigurableLanguage
,FieldStorageConfig
). Then this issue would have to be blocked on that one.On the other hand, I think many of these access control handlers will start to diverge over time. Their access control will get more refined. Particularly for complex and broadly/frequently used config entities like
Menu
.So I personally think it's not worth it.
Comment #17
tedbowOk. sounds good. RTBC!
Comment #19
catchCommitted 5e2e4a8 and pushed to 8.4.x. Thanks!
Comment #20
Wim LeersThanks!