The current behavior for Content Moderation is to return AccessResult::forbidden()
when update
access is checked for a moderated entity that does not have a valid transition to a different state.
This issue is not about whether this is a correct behavior ( that has been brought up here ), but the fact that it does so without providing a reason.
Due to the current lack of solid documentation (we are implementing workflows and lost a good two days figuring this requirement out), there should at least be a reason for forbidding access -- it's why this functionality exists and would make a HUGE difference in tracking down what is going on where.
I have attached a very simple patch that adds the reason Content Moderation: No allowed transitions exist.
, so that there is a hint to developers using $entity->access('update', $account,true);
. It is a (very) simple change that would have saved us at least a day of debugging our custom hook_node_access_records
et al.
Comments
Comment #2
hexblot CreditAttribution: hexblot as a volunteer commentedComment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI completely agree with this, content_moderation does alter access to entities in ways which aren't always totally obvious. Adding as much context as possibly to indicate what is going on is a great DX/REST improvement.
Doing a quick search, it looks like we forbid access to various things in the following places without providing a reason why:
content_moderation_entity_access
(as per the original issue summary).content_moderation_entity_field_access
, restricting the 'published' field, in favour of requiring a moderation_state instead.LatestRevisionCheck::access
, restricting access to to the latest-version route if entity has no pending revision.ModerationOptOutPublish/ModerationOptOutUnpublish
: restricting access to the publish/unpublish actions for the same reasons as #2.How would you feel about rejigging this issue to include helpful messages for all these scenarios too?
Comment #4
hexblot CreditAttribution: hexblot as a volunteer commentedRerolling patch to include reason messages for all
AccessResult::forbidden()
cases found.Thanks @Sam152 for the suggestion!
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWe're missing one in
LatestRevisionCheck::access
still. I also think we can safely remove the "Content Moderation:" prefix, so long as we make sure the messages are descriptive enough.I think for this message it's worth pointing out that it's the current user that doesn't have access to any valid transitions. Valid ones might exist generally speaking.
Maybe in this one we can mention what the alternative is too? Something like "The published field cannot be edited directly for moderated entities. Instead the moderation_state field should be used."
We don't have an action for moderation states, so sadly there is no alternative to these.
Comment #6
hexblot CreditAttribution: hexblot as a volunteer commentedChanges in attached patch:
LatestRevisionCheck::access
EntityAccessControlHandler::fieldAccess
(only core module I could find that includes messages)Regarding your points:
To my understanding, the check is not against the current user -- rather, the passed
$account
, hence the more generic message. Amended toNo valid transitions exist for given account.
nonetheless.Trying to follow the
EntityAccessControlHandler::fieldAccess
format for messages, since it appears to be the only one in core to include messages - and it does not propose resolution, just points out the reason for the current result. I do also feel that trying to recommend an approach might be presumptuous since other modules might alter the possibilities for resolution, but not the root cause.Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThe message seem generally a bit light on detail, but the points in #6 make sense and they are certainly better than what we already have. Setting NR for test bot.
Comment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLooks like the tests passed. I'll let someone else RTBC for a second opinion on the messages themselves.
Comment #9
timmillwoodAm I right in thinking these messages are not user visible, therefore using the word "entities" is ok?
If so, only one small nitpick:
I think this should be "Cannot edit the published field..."
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWhoops, didn't mean to update the status in #8.
Comment #11
hexblot CreditAttribution: hexblot as a volunteer commented@timmillwood These messages only appear when explicitly calling
$entity->access($op, $account,true);
(last boolean is optional andFALSE
by default, so only visible during debugging or similar).Rerolling the patch to include "small nitpick".
Comment #12
timmillwoodThanks @hexblot, looks fine to me now.
Comment #13
alexpottCommitted 4a85296 and pushed to 8.6.x. Thanks!