Don’t allow deleting revision translations in pending revisions

I tested again now, it looks like even though I after deleting the translation see it in the translation overview page I'm not able to view it anymore. When I click to view the translation I just see the original language. If I use the operation link to try delete the language a second time I'm in fact sent to a page to delete the original language (the whole node).

I've also made another observation that might be a little bit related to this. If I add a draft for a new translation to a node and look at it I'm also being sent to view the original language. I can see the draft inside the "Latest version" tab, but when I click "View" I see original language, and inside "Revision" tab I see revision for original language. It seems like the draft just inherit whatever it doesn't have from the original language. Maybe it would make sense to not display "view" tab and "revision" tab as the draft for a new translation doesn't have those thing yet. Just like hiding "Delete" button these things could be hidden too.

Still I think there should be a "Discard" button to be able to trash a draft you do not want to keep.

Still I think there should be a "Discard" button to be able to trash a draft you do not want to keep.

Agreed this would be ideal, but it's just nearly impossible to implement properly without a deleted flag for each revision translation. In fact right now when you create a new revision as a draft, the default revision has no entry for that translation. So right now in the storage a new draft translation and a removed translation are basically indistinguishable.

Here's an attempt to fix this while preserving the ability to delete translations: this patch is hiding the delete link only when dealing with a pending revision, which means that translations can be removed only in default revisions. This should be an acceptable compromise, since a pending revision is normally not accessible to regular users anyway.

I tested the patch, however I'm still able to see the delete link when dealing with a pending revision.
When I click delete it deletes the published version of the translation, but keep the draft revision.

After the published version is deleted I still see a "view" tab. When I click on that I see the node it it's original language, which to me seem a little bit strange.

Do you mean the delete tab?

Posting some additional test coverage. I have lingering failure I was not able to troubleshoot yet. @Wim Leers said he would look into it.

Updated IS

Still a few rough edges to smooth out, but this should be close to complete. Adding screenshots of the proposed solution.

@matsbla, it would be great if you could test this again.

@matsbla:

After the published version is deleted I still see a "view" tab. When I click on that I see the node it it's original language, which to me seem a little bit strange.

This is "by design": it's how core works right now, not something this patch is introducing, I believe. It should be the regular behavior when a translation does not exist yet.

Okay I get it now, I was looking at the edit form, there is still a delete button there. Earlier when I click on it I delete the published version of the translation, but in last patch I come to an "Access denied" page.

I'm a little bit confused about the solution for this issue. The issue is named "Draft revisions of translations can not be deleted", but after I apply this patch I'm not able to delete/discard a draft. With this approach to me it seems like I need to first publish a draft and first then I'm able to delete the translation. But I can't delete a draft and still keep current published version.

This is "by design": it's how core works right now, not something this patch is introducing

I understand that it is not introduced here, but is this really how it is intended to work? I find it quite confusing that I see another language at the "view" button, I would find it more logical if I'm not able to click a "view" button at all if there is no published translation to view. Anyway I just mentioned the detail as it seemed related earlier when I did the initial tests.

@matsbla:

Okay I get it now, I was looking at the edit form, there is still a delete button there. Earlier when I click on it I delete the published version of the translation, but in last patch I come to an "Access denied" page.

Yep, I forgot about that button, it will need to be hidden as well.

I'm a little bit confused about the solution for this issue. The issue is named "Draft revisions of translations can not be deleted", but after I apply this patch I'm not able to delete/discard a draft. With this approach to me it seems like I need to first publish a draft and first then I'm able to delete the translation. But I can't delete a draft and still keep current published version.

So the goal here is to implement something we can ship in 8.5. I agree the solution is suboptimal, but it always lets you make a translation unavailable to end users, which is the main goal after all. You can delete the draft by going to the revision list page, once you do that, you can delete the translation in a default revision, if you wish to do so.

This one should be ready for review.

Forgot to add a PHP doc. Attached also a test only patch.

Small clean-up.

@Wim Leers:

Great review, thanks!

This should address #23.

13: Because depending on the permissions, CT exposes different alternative routes in the UI: if a user has no edit permissions, a stripped down edit form or a translation deletion form is presented.

Another minor clean-up.

Generally looks fine, only a couple of minor nit picks.

I also wonder, would uploading a test-only patch help explain / prove the issue?

1. +++ b/core/modules/content_translation/src/Access/ContentTranslationDeleteAccess.php
   @@ -0,0 +1,116 @@
   +   *   The parametrized route.
   

   Should this be parameterized?

2. +++ b/core/modules/content_translation/src/Access/ContentTranslationDeleteAccess.php
   @@ -0,0 +1,116 @@
   +  public function checkAccess(ContentEntityInterface $entity) {
   

   This method seems a bit longer than it needs to be, but makes it nice and easy to read.

3. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
   @@ -109,12 +107,15 @@ public function access(Route $route, RouteMatchInterface $route_match, AccountIn
   +          // @todo Remove this logic once https://www.drupal.org/node/2945956 is
   +          //   fixed.
   

   Maybe open an issue postponed on #2945956: Allow removing translations in pending revisions to remove this logic, then replace the comment with a single line @todo Remove in https://www.drupal.org/node/#######.

1: Not sure what you are asking :)
3: I think #2945956: Allow removing translations in pending revisions is the place to remove those lines, reworded comments to clarify that.

Addressed #26.1: I didn't realize there was a typo.

Thanks @plach, looks good to me!

I'm afraid this patch makes more changes than I'm personally comfortable committing during the 8.5 RC phase. I'm leaving this as RTBC in case another committer is more comfortable with it.

From what I can tell, it would be ok to commit this or something like it into a patch release. It's not ideal for 8.5.0 to ship with Delete links that appear to work (all the way up to the final status message of "[French] translation of the Article [blablabla] has been deleted."), but don't actually end in a state where it's been deleted. However, as far as I can tell, there's no data loss or corruption or integrity violation: just a status message that wasn't accurate, so I think it's not that bad to fix this in 8.5.1 instead.

For me, it would be easier to commit this (between 8.5.0 and 8.5.1) if the following were split into separate issues:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -281,6 +282,13 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
   +      // Make sure we do not inadvertently recreate removed translations.
   +      foreach (array_diff_key($new_revision->getTranslationLanguages(), $translation_languages) as $langcode => $language) {
   +        if ($langcode !== $active_langcode) {
   +          $new_revision->removeTranslation($langcode);
   +        }
   +      }
   

   We need to be really careful with changes to ContentEntityStorageBase in patch releases (or during RC), since it's very prone to unintended consequences. So, would it be possible to split this into its own issue for more eyes on it?

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -576,7 +584,7 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   -    if ($display_warning && !$form_state->isSubmitted() && !$form_state->isRebuilding()) {
   +    if ($display_warning && $this->isMessageDisplayable($form_state)) {
   ...
   +  protected function isMessageDisplayable(FormStateInterface $form_state) {
   +    return !$form_state->getUserInput();
   +  }
   

   I don't understand yet why it's desirable to change this from checking submitted/rebuilding to checking user input. Could this be split into its own issue for clarity?

If someone else wants to commit this without needing the above split out, that's fine too.

Split the CESB changes out to #2949619: Removed revision translations may reappear when creating a new revision and made it critical, since I realized that that bug introduces data integrity issues. This is now postponed on that one.

I also reverted the unrelated changes to the message display, that were meaning to prevent the message from appearing in some edge cases (e.g. when uploading a file via AJAX or coming back from a node preview). I will create a separate issue for that.

I think it would be way better to commit this before 8.5.0 because, even if technically this bug does not introduce data integrity issues, allowing to delete translations in pending revisions will bring the DB in a state from which the CT UI can no longer work properly (and a future update adding a flag for deleted translations cannot deal with). For sure the fix included in this patch would not restore the UI functionality in those cases.

The blocker was committed, here's a reroll. Back to RTBC.

@effulgentsia suggested a fix in Slack

I committed #2949710: Pending revisions may become unavailable when untranslatable fields affect all translations which #34 conflicts with. Here's a reroll.

+++ b/core/modules/content_translation/src/Access/ContentTranslationDeleteAccess.php
@@ -0,0 +1,116 @@
+    if (!ContentTranslationManager::isPendingRevisionSupportEnabled($entity->getEntityTypeId(), $entity->bundle())) {

With #2949710: Pending revisions may become unavailable when untranslatable fields affect all translations committed, this now needs cache dependencies added to the access result. Here's a patch that does it one way. Not sure if there's a better way.

I think showing the warning on the edit form is strange. I went to edit, its not a warning about editing but about a tab the system is not showing. Why would I care, I am not here to delete? If we put up warnings about things you did not want to do, how do we expect people to read them (when they are actually useful)? :)

I think showing on the translation page may be more accurate, although you may not be going there to delete either, if you don't find the delete button in the tabs, I think you'll go there next.

I think it would be way better to commit this before 8.5.0 because, even if technically this bug does not introduce data integrity issues, allowing to delete translations in pending revisions will bring the DB in a state from which the CT UI can no longer work properly

Is this true with the current UI in HEAD? If so, what are the steps that surface the db being in a non-functional state?

(and a future update adding a flag for deleted translations cannot deal with). For sure the fix included in this patch would not restore the UI functionality in those cases.

I'm tentatively raising this issue to Critical for this. Even if HEAD's current UI is masking a fundamental db problem that occurs when you delete a translation with a pending revision, if it's true that the db still gets into a state that will create known future data loss problems for us, then we probably do need to get this into 8.5.0 or figure out what to do if we don't.

NW for #37. Unless a consensus emerges to commit #36 anyway, in which case set this back to NR or RTBC as appropriate.

FWIW, I reviewed #36 (and earlier iterations), and would feel comfortable committing it (to both 8.6.x and 8.5.x) once #36's interdiff is reviewed by someone and #37 is addressed. I'm heading to bed now though, and won't be up when the 8.5.0 commit freeze starts in a couple hours, so if another committer in a more convenient timezone feels comfortable committing it once those things are addressed, please do! Otherwise, I'll check in with the release managers tomorrow to see if it's eligible for commit during the freeze.

This addresses #37 and reworks the test to leverage the new ContentTranslationPendingRevisionTestBase class the was recently committed.

The interdiff form #36 looks correct to me.

UI looks good to me now. As good as it can get in this issue. :)

Thanks everyone. This was really close to not making it to 8.5.0.

Yay, thanks to all the people that spent (lunch ;) time on this!

This patch does not seem to be applied to the 9.x branch, why ?