Revert and Delete actions should not be available for the current revision

One note from a related slack thread, I'm not sure if an (only) access-based implementation for this is really the right thing. It's not really about access, it's about data consistency.

\Drupal\node\Controller\NodeController::revisionOverview() handles that directly in there, and I think the default implementation could too. That said, I suppose (also?) implementing it as an access check could help with exposed additional interfaces like jsonapi or something..

Additionally, it probably makes sense to throw an exception on the API level if trying to do it anyway.

Implementing #2 🐛 Revert and Delete actions should not be available for the current revision Fixed as this is blocking ✨ Add Media revision UI Fixed

I implemented the approach discussed in #2, and it will work without any BC breaks, to test it I used the newly created helper class for block content. If we don't want to rely on access based solution as stated in #4 🐛 Revert and Delete actions should not be available for the current revision Fixed I will need some help with how to implement that.

This will need test coverage, as previously tagged.

But tried replicating on 11.x with nodes and content blocks and I don't see the revert or delete for the current revision without using the patch. So adding steps to reproduce.

Steps to reproduce:
1. with the current patch applied, In \Drupal\block_content\BlockContentAccessControlHandler extend EntityAccessControlHandler instead of EntityAccessControlHandlerWithRevision.
2. You should be able to see the revert and delete button for recent revision for block.
3. Now revert this change and go back to the initial stage with just the patch applied.
4. you should not see the revert and delete button for latest revision.

I will write the test, once I get initial pass on this approach, that will save some time. Thanks.

Described this in #10 🐛 Revert and Delete actions should not be available for the current revision Fixed

So this issue currently doesn't exist unless it's setup?

1. with the current patch applied, In \Drupal\block_content\BlockContentAccessControlHandler extend EntityAccessControlHandler instead of EntityAccessControlHandlerWithRevision.

No, because we already have AccessResult::forbiddenIf($entity->isDefaultRevision() && $entity->isLatestRevision() in \Drupal\block_content\BlockContentAccessControlHandler::checkAccess, and we want to remove it from there.

I suggest we do this, as suggested by #2:

EntityAccessControlHandler::access already has some logic for if the Entity is an instance of RevisionableInterface, so maybe we could get away with just adding this to the existing access method, or maybe we move that existing logic into the new class that I described.

To me, this would reduce API surface and code duplication, and also protect all entity types from the data integrity issues caused by this bug. That might technically be considered a behavior-breaking BC change, but if the current behavior is capable of breaking entities and making them unusable, that's data loss and probably a sufficient justification for fixing the broken behavior.

I think it should be fine to do that in `access()` for two reasons.
1. We need to call the `checkAccess()` method at the start of every override of `checkAccess()` and check the returned result is forbidden, if yes then do an early return.
2. We are skipping the caching stuff only for the case where it is latest and default for delete or revert operation, in which case it's anyway going to be forbidden. So, I am not sure if that case is going to be a problem.

Yeah, checkAccess is usually not calling the parent and calling it and only respecting forbidden is very arbitrary.

I'm still not convinced that this is logic that should be done through the access API. At least we should additionally prevent this with an exception directly in the storage. See \Drupal\Core\Entity\EntityStorageBase::doPreSave() and \Drupal\Core\Entity\ContentEntityStorageBase::doPreSave() for existing examples that through exceptions if a save (or delete revision in this case) operation would cause data integrity issues.

This does not need extra test coverage IMHO, it's a refactoring of an existing check and already tested as the test fails in https://www.drupal.org/pift-ci-job/2714026 → show.

Addressed @Berdir's feedback and test fix.

Reviewed, this is not correct indeed. Not sure if my feedback was misunderstood, because what I asked for does in fact already exist and does not require a change. The tests added/changed here are either incomplete or incorrect about the default revision behavior if there are forward revisions.

The notes about missing/bogus test coverage around node revisions and the deleteRevision() check are just documenting my findings as I was going through this, it doesn't need to be done here. But it might make sense to have a follow-up to add better test coverage for deleteRevision() to avoid breaking this in the future.

For the node revision UI, we already have an issue to use the new generic code, which I guess will then also cover to unify node with block_content/media.

Reverted the change and created a follow-up.

The access check still has the same incorrect default AND latest check that is not correct. Since this is new code, we should make sure that is covered here with tests.

If we remove that default AND latest check, how are we going to achieve what we want to do in this issue? Or we want it to be just default?

> Or we want it to be just default?

Yes, this.

I messed up my formatting, but what I wrote previously is
> The same can also easily be done in the UI on the MR, and you get the delete/revert operations next to the current revision message.

So install content_moderation, enable it for custom blocks, then create a draft of a published block, then it must not be possible to delete the default revision.

And then either a UI or a kernel test (which doesn't need content moderation, it just needs to create an entity with setnewRevision(TRUE) + isDefaultRevision(FALSE)) where you verify that. Very much like the node issue you created, so that keeps working once we switch node to this.

I have changed it to just check default and on creating a draft of published custom block. I can see the revert and delete button, but I am not able to delete the published revision. That's what we are expecting, right? If yes, I will continue to write the test.

You should see delete and revert for the draft, but not the current revision, yes. Testing manually, I think this is also broken on blocks in HEAD, I guess you just moved that logic. If you compare it with node, it's different (and node is doing it correctly)

It is working as expected now I had missed something. Working on test.

Both functional tests looks good to me.
Q: Does this change require a CR?

I think this is ready now.

Some notes/summary:

* I'm changing it to a bug, because it's not just making the existing check in block_content generic, it's fixing it as well as it is currently wrong.
* Still feels a bit strange that the logic for this is part of access checking, but we have the API level check as well where we throw an exception (but no existing tests for that, but that's not in scope). The advantage of doing it in access means we kind of have an API for this that for example jsonapi could check too.
* Node does this in its controller currently, but that will change when node starts using the generic UI.
* The force revert stuff was bogus, even without the access check, you can't produce a data consistency issue due to the exception. I think that's why this is only a normal bugfix and not major. You could argue that the test coverage for it should be in the generic test entity type and not block_content, but I don't feel so strongly about it.

Caught a bug while testing this, that obviously needs test coverage (checking for revert or delete revision access when $return_as_object is FALSE)

Added test coverage.

You could argue that the test coverage for it should be in the generic test entity type and not block_content, but I don't feel so strongly about it.

I think it does make more sense for the test coverage to be in the generic test entities. Having the test coverage in the block_content module for something which is being made a generic implementation feels weird.

Catching up and reading the comments seems like the consensus is to move those tests to be generic.

If there's an issue with block_content should that be fixed first before this?

I think we can rely on this test coverage because we will be adding similar tests for node and media also after they start using generic UI. Anyway, we can't skip test coverage for node, media, and block content, so if we write a test for generic it will get repeated.
If there is any strong reason for writing tests in generic, we can do that.

I think we can rely on this test coverage because we will be adding similar tests for node and media also after they start using generic UI. Anyway, we can't skip test coverage for node, media, and block content, so if we write a test for generic it will get repeated.
If there is any strong reason for writing tests in generic, we can do that.

We already have the test coverage we need in, \Drupal\FunctionalTests\Entity\RevisionDeleteFormTest::testAccessDeleteDefault and \Drupal\FunctionalTests\Entity\RevisionRevertFormTest.
Other than that, I think we should keep the changes to the block content's test, according to 📌 Improve test coverage around node revisions and the deleteRevision() Active .

Great work @yash.rode this is ready to go again.

I think it would be great to also get an issue summary update that describes the bug we are fixing, and the changes we are making.

If we need to make changes to code outside of the changes introduced here, it sounds like that the test coverage might not be sufficient. Since we are introducing new access controls to the generic entity access control handler, it should be tested outside of the implementations.

Looks like the test coverage introduced in ✨ Implement a generic revision UI Fixed has a mistake in a critical part which makes it fail to test what it's supposed to 🙈 I'll work on this a bit. Here's a patch that should fail.

New tests look good, IS is updated.

Nice catch on that test bug!

Looks like I forgot to unassign this. Thanks for the issue summary update and for moving this to needs review @acbramley!

Issue summary reads clear now.

Think this is ready!

Committed 117e716 and pushed to 11.x. Thanks!

Technically, I think this could be backported but it's probably safer to do this in a minor only due to the changes in the access control handler.

lauriii → closed merge request !4359

Automatically closed - issue fixed for 2 weeks with no activity.