Add canPublish check for ChangeSet #384
Conversation
mark-a-j-adriano
force-pushed
the
dev-owns-canpublish-check
branch
from
ec741ca to
287bd64
Compare
| $member = Security::getCurrentUser(); | ||
|
|
||
| // Check if object has canPublish set to true | ||
| if ($object->canPublish($member)) { |
There was a problem hiding this comment.
We shouldn’t add this kind of logic here, as it’ll potentially block programatic publishing (e.g. a CLI task where no user is logged in)
There was a problem hiding this comment.
ChangeSetItemalready has its owncanPublishmethod that checks the record object's canPublish, so it must be something else going on (if something is actually going on).
Nope, ChangeSetItem canPublish method is not called.
silverstripe-versioned/src/ChangeSetItem.php
Line 307 in 4baa7b4
Already acts on the asset
There was a problem hiding this comment.
@kinglozzer -> not sure where to put the logic if it is not the correct place. (In our use case this might suffice)
There was a problem hiding this comment.
I think for your use case I’d probably remove the image from $owns and add some logic in onAfterPublish() to check the date and publish if applicable
There was a problem hiding this comment.
That would be a workaround for Mark, but I think this is a legit bug where it is reasonable to anticipate the permissions are respected, or we openly document that ownership cascade publishing trumps individual object permissions.
There was a problem hiding this comment.
I think this is a legit bug where it is reasonable to anticipate the permissions are respected
I agree
There was a problem hiding this comment.
Yep sorry I meant as a workaround! Will post my thoughts in the issue #383
|
|
|
I'm going to close this PR for now. As mentioned in the issue, this can't be resolved until we're ready to start working on a new major release. |
Maybe the solution for #383