Deleting an entity translation from the UI deletes the whole entity

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
   +    $entity = $this->getEntity();
   +
   +    // Make sure that deleting a translation does not delete the whole entity.
   +    if ($entity->language()->getId() != $entity->getUntranslated()->language()->getId()) {
   +      $entity->getUntranslated()->removeTranslation($entity->language()->getId());
   +      $entity->getUntranslated()->save();
   +    }
   +    else {
   +      $entity->delete();
   +    }
   

   Just from looking at that it feels super wrong that a delete entity form, cares about translations ... kinda and also has some special behaviour.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -594,9 +601,9 @@ public function entityFormSourceChange($form, FormStateInterface $form_state) {
   +    $is_translation = $entity->language()->getId() != $entity->getUntranslated()->language()->getId() ? TRUE : FALSE;
   

   You really don't need the ternary

It feels for me like clicking delete should always describe that we delete the entire thing.

I agree with dawehner here. What about having "Delete" and then "Delete (only this translation)".

@Bojhan, do you mean "Delete" in the delete form for the source language (the one that also has a warning now) and "Delete (only this translation)" in the delete form of the translation?

Edit: If we look at the "after" screenshots from the IS, that would mean:

"Delete (all translations)" => "Delete"
"Delete (this translation)" => "Delete (only this translation)"

I think we talk about the actual node form?

Should there be some way to delete the original translation as well?

I talked to YesCT about this, we agreed that the current approach is OK. Having deleting on only the source.

But we should do some optimisations:

• The deleting translation button should be "Delete (romanian translation)"
• On deleting the source node, it should list all the things you are about to delete (like we do on the module list)
• We shouldn't use a message here, that's not good practice on a confirm page.

I think we should see where we get redirected to after the deletion of one translation.

I suspect/hope it redirects to view on the actual node.
Then, someone who might be wondering why the node is not gone, would pick the delete tab there, and it would there delete everything.

If it happens like that, then I think we do not as much need any words on the translation delete which says how to delete the whole thing, cause they will discover it and then they will know.

Agreed with the optimisations Bojhan outlines. I don't think we need parentheses though.
So just have "Delete Romanian translation".
And, yes to what YesCT said as well.

Working on this now at LA sprint. I've replicated the issue.

First stab at this. This tackles point 1 in #12.

Also, after deleting a translation, it redirects to the front page in the translation that was just deleted (so if you delete German, it redirects to mysite.com/de). I think this would be less confusing if we made it redirect to the node view of the translation that was just deleted.

1. --- c/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   
   @@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   

   so, should we only worry about nodes or more generally content?

   OR more specifically, should we leave the content and node alone and make a submitForm() in a TranslationDeleteForm .. ?

2. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   --- c/core/modules/node/src/Form/NodeDeleteForm.php
   +++ w/core/modules/node/src/Form/NodeDeleteForm.php
   
   +++ w/core/modules/node/src/Form/NodeDeleteForm.php
   +++ w/core/modules/node/src/Form/NodeDeleteForm.php
   @@ -19,12 +19,30 @@ class NodeDeleteForm extends ContentEntityDeleteForm {
   
   @@ -19,12 +19,30 @@ class NodeDeleteForm extends ContentEntityDeleteForm {
       * {@inheritdoc}
       */
      public function submitForm(array &$form, FormStateInterface $form_state) {
   ...
   +    parent::submitForm($form, $form_state);
   +
   +    $form_state->setRedirect('<front>');
   

   this of overriding the addition of submitForm() on the ContentEntityDeleteForm class.

   which is why it is redirecting to the front. and not the redirect put in that other submitForm().

I've been talking with @YesCT about this. The behavior occurring is confusing, because we're overriding the node delete tab to delete a translation (so it doesn't delete the whole node). That explains why it's not getting routed back to the translation page of the node where the translation has been deleted. When you delete a translation on the translation tab, it properly removes the translation and sends the user back to the translation page.

What it looks like we need to do, is in the DeleteForm submit hook, check if we're deleting a translation, and if so, then set the redirect to the view node page of the default translation.

#5.1 is still a problem IMHO. This is a work around not a proper fix. Maybe we can ping @Berdir or @yched about it.
RE: #20 Let's not touch the ui here at all then because it seems to me that we need a proper discussion around that. I'd suggest let's not run into fixing it.

Yeah I have read #7.1 before that's the reason I suggested to ping @berdir or @yched but any ways if TranslatableInterface is your reason then why not add a new interface(ContentEntityTranslatableFormInterface or something like that) to ContentEntityConfirmFormBase and update the if statement something like this

if (!$entity->someMehtodOnContentEntityConfirmFormBase()) {
  $entity->delete();
}

and move

      $entity->getUntranslated()->removeTranslation($entity->language()->getId());
      $entity->getUntranslated()->save();

to the above method.
How does this sound?

As pointed out by @amateescu, I don't think it makes sense to separate translation handling in a dedicated class just for forms, as it doesn't increase the overall coupling, which is already very tight.

I'd address the UX remarks here, though as the seem easy to fix, namely:

• make sure a translation deletion redirects to the entity view page
• replace the message with a list of deleted translations
• adjust the submit button labels

Quick code review, nice work, no biggies:

1. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +    if ($entity->language()->getId() != $entity->getUntranslated()->language()->getId()) {
   +      $entity->getUntranslated()->removeTranslation($entity->language()->getId());
   +      $entity->getUntranslated()->save();
   

   Can we store the untranslated object in a variable instead of retrieving it multiple times? It makes the code slightly more readable.

2. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -225,6 +226,12 @@ public function getSourceLangcode(FormStateInterface $form_state) {
   +    if ($form_state->getFormObject() instanceof ContentEntityDeleteForm) {
   

   We should probably add an elseif check and bail out if we are on a custom entity form having a different operation from edit/default.

3. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -594,9 +601,9 @@ public function entityFormSourceChange($form, FormStateInterface $form_state) {
          drupal_set_message(t('This will delete all the translations of %label.', array('%label' => $entity->label())), 'warning');
   

   I think it's fine to list the translations that would be deleted instead, as suggested above. I'd move the related code to the entity form, so it works even if CT is not enabled.

4. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +    $form_state->setRedirectUrl($this->getCancelUrl());
   

   I'm not sure about this: won't it point to the entity itself, even if it has just been deleted, if a collection link template is not defined? Should we fix ::getCancelUrl() to account for that and return <front> in that case?

5. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +    drupal_set_message($this->getDeletionMessage());
   +    $form_state->setRedirectUrl($this->getCancelUrl());
   +    $this->logDeletionMessage();
   

   We should have different log/messages in the two cases, I guess.

6. +++ w/core/modules/node/src/Form/NodeDeleteForm.php
   @@ -19,12 +19,30 @@ class NodeDeleteForm extends ContentEntityDeleteForm {
      public function submitForm(array &$form, FormStateInterface $form_state) {
   -    $this->entity->delete();
   -    $this->logger('content')->notice('@type: deleted %title.', array('@type' => $this->entity->bundle(), '%title' => $this->entity->label()));
   +    parent::submitForm($form, $form_state);
   +
   +    $form_state->setRedirect('<front>');
   +  }
   

   If what I suggested above makes sense this would go away.

7. +++ w/core/modules/node/src/NodeTranslationHandler.php
   @@ -53,13 +55,19 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +        $form['actions']['submit']['#value'] .= ' ' . ($is_translation ? t('(@language translation)', array('@language' => $entity->language()->getName())) : '');
   

   Let's accomodate this too, since it's quite easy: the suggestion is replacing it with "Delete @language translation" if I'm not mistaken.

+++ w/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -225,6 +226,12 @@ public function getSourceLangcode(FormStateInterface $form_state) {
+    // Only alter entity edit forms.
+    if ($form_state->getFormObject() instanceof ContentEntityDeleteForm) {
+      $this->entityFormDelete($form, $form_state);
+      return;
+    }

The comment contradicts the actual code a bit, we still do something on delete forms (even if, when you look into the main class implementation, it happens to just be a dsm())

Also, entityFormDelete() phpdoc is a submit callback (and that's what its phpdoc says it is), so it feels weird to directly reuse that as part of a form alter ? It seems overrides in base classes could totally trust the phpdoc and do "submitty" stuff in there that would be ill-advised at alter time ?

Last, wouldn't it be a bit more direct if content_translation_form_alter() took care of "if that's delete form, do this, else do that", instead of deferring all cases to entityFormAllter() ?

+++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
@@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
+    drupal_set_message($this->getDeletionMessage());

+++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
@@ -14,4 +16,25 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
+    // Make sure that deleting a translation does not delete the whole entity.
+    if ($entity->language()->getId() != $entity->getUntranslated()->language()->getId()) {
+      $entity->getUntranslated()->removeTranslation($entity->language()->getId());
+      $entity->getUntranslated()->save();
+    }
+    else {
+      $entity->delete();
+    }
+
+    drupal_set_message($this->getDeletionMessage());

Also, the same message (in the default implementation, never overriden : "The @entity-type %label has been deleted") will be displayed no matter what ? That sounds like it could give users a cold sweat, don't we need a dedicated message for the "just deleted a translation" case ?

Here's a patch addressing most of the concerns. I'm not sure what needs done for the 2nd request in #25 I haven't moved anything yet (like the request in 3 to move logic to the entity form). And I haven't fixed the redirect to not go to the page. 4 and 6 still need addressed as well in that comment. Setting needs review to make sure the changes thus far don't break tests.

I'm not sure what needs done for the 2nd request in #25

I was suggesting to just add a check that $this->getOperation() == 'edit' or $this->getOperation() == 'default' or return immediately.

1. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -24,8 +24,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    if ($entity_language_id != $entity->getUntranslated()->language()->getId()) {
   +      $entity->getUntranslated()->removeTranslation($entity_language_id);
          $entity->getUntranslated()->save();
   

   Probably I didn't explain myself: I was suggesting to store in a variable the result of $entity->getUntranslated().

2. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -603,8 +603,17 @@ public function entityFormSourceChange($form, FormStateInterface $form_state) {
   +      drupal_set_message(t('This will delete the following translations: @languages',
   +        array(
   +          '%label' => $entity->label(),
   +          '@languages'   => $languages,
   +        )), 'warning');
        }
   

   I think the suggestion was to list translations as a bullet list in the form page content, not as a message.

3. +++ w/core/modules/node/src/NodeTranslationHandler.php
   @@ -66,7 +66,7 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +        $form['actions']['submit']['#value'] .= ' ' . ($is_translation ? t('@language translation', array('@language' => $entity->language()->getName())) : '');
   

   Concatenating translated strings is not a good practice: we should completely replace the value with $this->t('Delete @language translation', ...).

With the latest patch, I think the only thing remaining is getting rid of the drupal_set_message with the concatenated language strings and replacing with a bullet list in the form. Also, possibly moving the logic to entity form from ContentTranslationHandler mentioned in #25.

Switching list of languages being deleted to bulleted list.

Thank you for your work on this.

Concatenating translated strings is not a good practice: we should completely replace the value with $this->t('Delete @language translation', ...).

I like that wording better, but this is introducing inconsistencies with the following code:

if ($status_translatable && !$is_delete_form) {
  foreach (array('publish', 'unpublish', 'submit') as $button) {
    if (isset($form['actions'][$button])) {
      $form['actions'][$button]['#value'] .= ' ' . ($status_translatable ? t('(this translation)') : t('(all translations)'));
    }
  }
}

I think it would be better to improve them all at the same time. We could open a follow up after closing this one. The UX still needs to be improved in many places but I agree with @amateescu that we don't need to postpone a critical issue for that.

Saw a manual demo of it. Looks really great.

I think adding a phrase before the order list like:

This will delete the translations:

would be nice.

+    if (count($entity->getTranslationLanguages()) > 1 && !$is_translation) {
+      $form['languages'] = array(
+        '#theme' => 'item_list',
+        '#items' => $languages,
+      );
     }

This is a bit confusing because is just displaying a list of languages, without an indication that these are translations about to be deleted.

Adding markup above the list of items.

Last patch "deleting_a_translation-2486177-35.patch", working as expected, however the text is confusing, it should say "Are you sure you want to delete the <LANGUAGE> content Test 1 page?" e.g. "Are you sure you want to delete the French content Test 1 page?" not https://www.evernote.com/shard/s183/sh/6216f7ba-7507-41f7-be33-221ecd051...

I confirm #37 happens when going to /secondary-language/node/<nid>/delete of a node translated to that language. It displays the form to delete a translation but the header "Are you sure you want to delete the content..." is not consistent with it.

Just some random thoughts.

1. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,32 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +      $form_state->setRedirectUrl($entity->urlInfo('canonical'));
   

   Wait, shouldn't this use $entity_untranslated as otherwise a specific language would be used?

2. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -225,6 +226,15 @@ public function getSourceLangcode(FormStateInterface $form_state) {
   +    elseif (!($form_state->getFormObject()->getOperation() == 'edit' or $form_state->getFormObject()->getOperation() == 'default')) {
   

   nitpick: we use || not or. Can we document why we stop here?

3. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -593,11 +603,20 @@ public function entityFormSourceChange($form, FormStateInterface $form_state) {
   +  function entityFormDelete(&$form, FormStateInterface $form_state) {
   

   If we change this line, can we make it as protected?

4. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -593,11 +603,20 @@ public function entityFormSourceChange($form, FormStateInterface $form_state) {
   +    $is_translation = $entity->language()->getId() != $entity->getUntranslated()->language()->getId();
   

   Is that a functionality which would be helpful on entity itself? We do that more than just once place already in that patch.

5. +++ w/core/modules/system/src/Tests/Entity/EntityFormTest.php
   @@ -40,6 +44,16 @@ function testFormCRUD() {
   +  function testMultilingualFormCRUD() {
   

   Let's add public, even if the other ones don't do yet.

6. +++ w/core/modules/system/src/Tests/Entity/EntityFormTest.php
   @@ -85,12 +99,53 @@ protected function doTestFormCRUD($entity_type) {
        $this->container->get('entity.manager')->getStorage($entity_type)->resetCache();
   -    return current(entity_load_multiple_by_properties($entity_type, array('name' => $name)));
   +    $entities = entity_load_multiple_by_properties($entity_type, array('name' => $name));
   +    return $entities ? current($entities) : NULL;
   

   Given that we use the storage already, do you mind using ::loadByProperties as well?

Addressing #40.

As for #37, fixing that text could be difficult. It's coming from Drupal\Core\Entity\EntityDeleteFormTrait method getQuestion(). Conditionally overriding that when a translation is being deleted vs. when a node is being deleted could provide to be rather difficult. I'm open for suggestions if anyone has any guidance.

fixing syntax of loadByProperties() method (no entity_type param required).

Just some small points.

1. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,32 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +    if ($entity->language()->getId() != $entity_untranslated->language()->getId()) {
   

   I guess we should be able to use isTranslation there?

2. +++ w/core/lib/Drupal/Core/Entity/Entity.php
   @@ -124,6 +124,19 @@ public function enforceIsNew($value = TRUE) {
      /**
   +   * Checks if entity is a translation.
   +   *
   +   * @return boolean
   +   *   true if it is a translation.
   +   */
   +  public function isTranslation() {
   

   We should add isTranslation() to TranslatableInterface and move this implementation to ContentEntityBase

This should pass tests again. Thanks @dawehner for the tip on using $entity->getEntityTypeId().

+++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
@@ -14,4 +16,32 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
+      $form_state->setRedirectUrl($entity_untranslated->urlInfo('canonical'));
+      $this->logger('content')->notice('@type: @language translation deleted %title.', ['@type' => $entity->getEntityTypeId(), '%title' => $entity->label(), '@language' => $language_name]);

Actually, I think you want to use getEntityType()->getLabel() here, that gives you the user-readable label (Content, User, Term, ..) and not the machine name (node, user, taxonomy_term, ...)

@Berdir, that makes sense. Updated patch & using the human readable label now.

This one should pass tests again. Also moving some stuff around per @dawehner.

@piyuesh23 can you attach an interdiff showing your changes you made? Want to make sure I get them added to the work I've been doing. If you're using git, just a git show of your most recent commit outputted to interdiff.txt should suffice.

@disasm, Adding interdiff here for the patch in #50.

Last 2 test failures being fixed here. Overriding node delete form so it redirects to front page when deleting nodes.

1. +++ w/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -260,6 +260,17 @@ public function isDefaultTranslation() {
   +      return true;
   ...
   +    return false;
   

   Should we TRUE|FALSE, sorry :(

2. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,32 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +      $this->logger('content')->notice('@type: @language translation deleted %title.', ['@type' => $entity->getEntityType()->getLabel(), '%title' => $entity->label(), '@language' => $language_name]);
   

   Just a nitpick, the logging message is a little bit influent.

addressing #60.

The latest patch does address the issues from comment #60.

The tests that are added seem to do what the issue summary suggests it should.

+++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
@@ -14,4 +16,36 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
+      drupal_set_message(t('The @language translation has been removed.', array(

+++ w/core/modules/node/src/Form/NodeDeleteForm.php
@@ -18,13 +18,30 @@ class NodeDeleteForm extends ContentEntityDeleteForm {
+    return $this->t('@type %title has been deleted.', array(

Should $this->t() be used in ContentEntityDeleteForm just as it is in NodeDeleteForm?

The patch looks much batter now some nits to fix and we are good to go.

1. +++ w/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -260,6 +260,17 @@ public function isDefaultTranslation() {
   +
   +
   

   Extra whitespace.

2. +++ w/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -14,4 +16,36 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +      $this->getEntity()->delete();
   

   We can just use $entity->delete().

3. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -225,6 +226,15 @@ public function getSourceLangcode(FormStateInterface $form_state) {
   +    if ($form_state->getFormObject() instanceof ContentEntityDeleteForm) {
   ...
   +    elseif (!($form_state->getFormObject()->getOperation() == 'edit' || $form_state->getFormObject()->getOperation() == 'default')) {
   

   Let's create $form_object = $form_state->getFormObject(); before if statement.

4. +++ w/core/modules/node/src/Form/NodeDeleteForm.php
   @@ -18,13 +18,30 @@ class NodeDeleteForm extends ContentEntityDeleteForm {
       * {@inheritdoc}
   ...
   +   * {@inheritdoc}
   

   Needs proper function docs now.

Addresses #37 (switches title when deleting a translation.

@jibran which methods need docs in #4 (the included part doesn't make it clear).

Patch addresses everything else in #62 and #63.

nm it's fine. Thanks for the fixes @disasm. I think this is ready. Let's wait for @plach to RTBC.

if ($entity->isTranslation()) {

Would that be the same than doing:
if (!$entity->isDefaultTranslation()) {?

I say that because that function already exists, and I think is implemented better (uses the language and does not call any method):

  public function isDefaultTranslation() {
    return $this->activeLangcode === LanguageInterface::LANGCODE_DEFAULT;
  }

+    foreach ($entity->getTranslationLanguages() as $language) {
+      array_push($languages, t('@language', array('@language' => $language->getName())));
+    }

From PHP documentation:

Note: If you use array_push() to add one element to the array it's better to use $array[] = because in that way there is no overhead of calling a function.

i.e.

$languages[] = t('@language', array('@language' => $language->getName()));

     $status_translatable = NULL;
+    $is_delete_form = $form_state->getFormObject() instanceof ContentEntityDeleteForm;
     // Change the submit button labels if there was a status field they affect
     // in which case their publishing / unpublishing may or may not apply
     // to all translations.
@@ -53,13 +55,20 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
           $status_translatable = $definition->isTranslatable();
         }
       }
-      if (isset($status_translatable)) {
+      if (isset($status_translatable) && !$is_delete_form) {

Is not your code and is minor, but now that you are changing it, I would remove the useless isset(). Also, $status_translatable could be initialized to FALSE instead.

(Actually, is not that minor. If $definition->isTranslatable() returns a boolean (and it should), isset() will always return TRUE.)

No answer on #26 ?

per #67 using isDefaultTranslation() instead of isTranslation. Removing the new method. Tested locally, and behavior hasn't changed. Also, only setting <front> on NodeDeleteForm when isDefaultTranslation().

I only had a quick look...

------

+++ w/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -329,7 +329,7 @@ public function bundle() {
   /**
-   * {inheritdoc}
+   * {@inheritdoc}

this is an unrelated change in a file not otherwise touched by this patch. please revert this part.

------

Also, the breadcrumbs are now not showing in the "after" screenshots. and the vertical spacing looks missing.

Please embed the after screenshots in the issue summary, near the before screenshots.

I just created #2488156: Fix typos in @inheritdoc to address the issue in #76

reverted #76. I think I accidentally introduced that one when I added the method isTranslation() we aren't using anymore.

Manually tested this and it works great, just a couple of remaining remarks:

1. +++ w/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -602,6 +605,33 @@ function entityFormDelete($form, FormStateInterface $form_state) {
   +    if ($entity->isDefaultTranslation()) {
   +      $languages = [];
   +      foreach ($entity->getTranslationLanguages() as $language) {
   +        $languages[] = $language->getName();
   +      }
   +
   +      if (count($languages) > 1) {
   +        $form['entity_deletes'] = array(
   +          '#theme' => 'item_list',
   +          '#title' => $this->t('The following @entity-type translations will be deleted:', [
   +            '@entity-type' => $entity->getEntityType()->getLowercaseLabel()
   +          ]),
   +          '#items' => $languages,
   +        );
   +
   +        $form['actions']['submit']['#value'] = $this->t('Delete all translations');
   +      }
   +    }
   +    else {
   +      $form['actions']['submit']['#value'] = $this->t('Delete @language translation', array('@language' => $entity->language()->getName()));;
   +    }
   

   I'm confused: why need these UX improvements to be provided by CT, given that all the rest is implemented generically by the entity form? If translations were created programmatically without CT, we would be missing the UX benefits added here.

2. +++ w/core/modules/node/src/Form/NodeDeleteForm.php
   @@ -18,13 +18,44 @@ class NodeDeleteForm extends ContentEntityDeleteForm {
   +      $form_state->setRedirect('<front>');
   

   As I wrote above, EntityDeleteFormTrait::getCancelUrl() is broken when the entity type does not define a collection link template, as it will try to redirect to the deleted entity canonical URL, which will always return a 404. We should return <front> in that case instead and remove the special casing from the node delete form.

@amateescu++ :-)

+++ w/core/modules/node/src/Form/NodeDeleteForm.php
+++ w/core/modules/node/src/Form/NodeDeleteForm.php
@@ -18,13 +18,44 @@ class NodeDeleteForm extends ContentEntityDeleteForm {

BlockContent also supports translatable entities. Should we be seeing changes in this patch for BlockContentDeleteForm too?
I note that the delete form there warns about removing placed instances.

Very very close!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -23,6 +23,39 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +      $languages = [];
   +      foreach ($entity->getTranslationLanguages() as $language) {
   +        $languages[] = $language->getName();
   +      }
   +
   

   This should go into the if branch below.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -23,6 +23,39 @@ class ContentEntityDeleteForm extends ContentEntityConfirmFormBase {
   +        $form['entity_deletes'] = array(
   

   Ubernitpick: what about deleted_translations?

3. +++ b/core/modules/content_translation/content_translation.module
   @@ -280,11 +280,8 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
   +    if ($form_object->getOperation() != 'delete') {
   

   I think we can safely merge this test in the if above.

Awesome!

I looked over this and this doesn't look like it'll need a change record. Updating issue summary since reviews are done.

The patch is looking great for me!

Really excellent work on this, everyone!

Tested this with a node with an English (source)/French translation (node titles: hello/bonjour), both with the delete button/tab on the two translations, as well as the admin/content page.

When attempting to delete the source translation (in my case, English), either from admin/content or from node/1/delete, you get this:

All is good.

If I hit the delete tab on the French translation, I get:

All is good again.

A couple of inconsistencies I found though:

1) If I hit the "Supprimer la traduction" button on the "bonjour" node form (as opposed to the "Supprimer" (Delete) tab, I get a slight variation, with no primary button styling:

That is confusing. Are there two delete forms for some reason?

2) When using Delete option within contextual links for "bonjour", it asks if I want to delete "hello" and then shows the "delete all translations" warning. Expected behaviour: Would only ask if I wanted to delete the French translation.

3) When attempting to delete "bonjour" from admin/content, the deletion form points to "hello" instead, and does not warn that all translations will be deleted, even though they will:

So I think there's still a bit more left to do here.

@webchick: looking at your screenshots I see "translation of the contenu" (note last word). I did not find that string in the patch and/or core, so not sure how that ended up there, just noting.

Reverted #75. These two functions do very different things.

@amateescu: right, what tripped me up is it was in the middle of an English sentence, but I see it comes from the entity type.

Right, I realize we need translation-aware entity operations in order to delete "bonjour" and not "hello" but since I am deleting "hello," and "hello" has translations, why is it not warning me that I'm going to blow away my other translations like it would if I hit delete on the node form directly?

Not being warned when you're about to delete translations you didn't realize you're going to delete I thought was the whole point of this issue? So, yes? :)

Ah, perfect. :) Will test a bit later today, thanks!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -260,6 +260,13 @@ public function isDefaultTranslation() {
+    return $this->language()->getId() != $this->getUntranslated()->language()->getId();

This should be actually equivalent to !CEB::isDefaultTranslation(), not sure what difference you are referring to: the ::language() method returns a language object corresponding to the current active language code, which is LanguageInterface::LANGCODE_DEFAULT for the default translation, so the actual default language code needs to be retrieved. ::isDefaultTranslation() allows to perform a lighter check but the logic behind is the same.

2) When using Delete option within contextual links for "bonjour", it asks if I want to delete "hello" and then shows the "delete all translations" warning. Expected behaviour: Would only ask if I wanted to delete the French translation.

I think this is a separate bug, I'm able to reproduce it even with the last patch: it seems contextual links' language is always the one of the available translation whose language has the heaviest weight, instead of matching the translation language.

Actually the bug exists in HEAD too: #2488940: Contextual links are broken on multilingual sites.

This should be actually equivalent to !CEB::isDefaultTranslation(), not sure what difference you are referring to: the ::language() method returns a language object corresponding to the current active language code, which is LanguageInterface::LANGCODE_DEFAULT for the default translation, so the actual default language code needs to be retrieved. ::isDefaultTranslation() allows to perform a lighter check but the logic behind is the same.

I was referring to !isDefaultTranslation() vs isTranslation(). I asked in #67 if they were equivalent, but clearly they are not.

The method is called ::isTranslation(), the logic behind it aims to figure out whether we are (not) dealing with the default translation object, the default translation object has 'x-default' as active language, thus !::isDefaultTranslation() gives us exactly the same result as ::isTranslation().

'x-default' is just a shortcut to designate the entity default/original/source language that allows to change it without having to mess with internal keys and so on. If you have a look to the ContentEntityBase::language() method, you'll see that the active language is returned, unless we are dealing with the default translation, in which case 'x-default' is converted to the actual value before returning it.

Because the delete form provided by CT does not have all the improvements that went into the regular delete form.
...
@plach, should we just remove that one or update it?

In #2476563: Entity operations links tied to original entity language, ignore everything else we are planning to use it as a fallback for users not having edit permissions on the entity, so I guess ContentTranslationDeleteForm could just extend ContentEntityDeleteForm and override the submit handler to avoid deleting the whole entity. We could additionally introduce a validation check to block submission in case we are dealing with the default translation, in fact this could happen only by manually fiddling with the browser address bar.

At least from my point of view, it seems pretty clear that its the exact opposite. In the beginning I would have started with the other way round for the method,
but at that point in time, its not worth to change the API, I think.

In the beginning I would have started with the other way round for the method,
but at that point in time, its not worth to change the API, I think.

We have an ::isDefaultRevision() method :)

We have an ::isDefaultRevision() method :)

Fair, but I just think it would have been easier the other way round.

Tested patch with instructions provided and it works well. Clarity on wording is much improved.

I have tested this too, and it works.

FWIW as someone new to this issue (and multi-lingual in general) I found it a bit confusing because you only get the "Delete this translation only" option if you are at the /language/node path (e.g. /cs/node/1/edit or cs/admin/content). So if you are at /admin/content and you click edit then delete, or use the drop down button from admin/content, you can only delete both. So it might be worth a follow up issue to clarify that, maybe just in the help text on the source node delete page, to say that you can delete single translations via the language-specific sub-path.

Would love plach to take one more look at this.

Actually just testing a bit further, I believe there is a bug that is not related to this specific issue.

If you are at admin/content and you click edit on a translation, it should take you to [language]/node/nid/edit but right now if you click edit on the translation it takes you just to node/nid/edit. I have created #2489602: Edit/delete links for entity translations link to source node, not translation for this issue.

Thanks @amateescu, I've some time here at the sprint and I'm addressing #111.

This takes care of unifying the CT deletion form.

As a bonus, this unties redirect URLs and cancel URLs, whose coupling was creating some problems.

+++ b/core/modules/content_translation/src/ContentTranslationHandlerInterface.php
@@ -7,6 +7,7 @@
+use Drupal\Core\Entity\ContentEntityInterface;

Not needed anymore.

Looks fine for me!

This should fix failures and address #123.

Tested and works fine.

+1 to RTBC.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
@@ -7,11 +7,138 @@
+  use EntityDeleteFormTrait {
+    getQuestion as traitGetQuestion;
+    logDeletionMessage as traitLogDeletionMessage;
+    getDeletionMessage as traitGetDeletionMessage;
+    getCancelUrl as traitGetCancelUrl;
+  }

wow, this is nice. didn't know that we can do like this.

I've tested patch from #128 and it works as expected. The form is now far more intuitive.

@Adam Clarey and @Saphyel, thanks for testing! One note for next time is that it's always helpful to document the specific steps you used for testing and what the specific results were. See @webchick's work in #94 for a great example.

The summary, screenshots, integration test coverage, etc. are all great

I'm in the process of reviewing this and I must admit the sorta-crazy around EntityDeleteFormTrait is snarling my brain. I read over #1728804: Introduce (Content)EntityDeleteForm and children to handle entity deletions for some background on why things are the way they are, but I'm still unclear on why, for example, ContentEntityConfirmFormBase is a thing (it seems only used for deletions), and why we don't just do away with EntityDeleteFormTrait at this point (when we're overriding it so much) and just put the respective versions of the methods in ContentEntityDeleteForm and EntityDeleteForm (which are the only usages). (For others' reference, ContentEntityDeleteForm does not extend EntityDeleteForm; and EntityDeleteForm has only config-specific stuff, which also confused me quite a bit.) Is there a reason to keep the trait other than for BC? Because then we could do away with the trait overriding cleverness. (The rest of the weird is probably out of scope.) Setting NR for feedback on that because I feel like the sorta-confusing inheritance around this is a bit potentially fragile and maybe how this bug occurred in the first place.

A couple other minor questions and notes I had:

1. +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php
   @@ -73,13 +73,18 @@ public function __construct(EntityManagerInterface $manager, LanguageManagerInte
   +      // Translation operations cannot be performed on the default translation.
   +      if ($operation != 'create' && $language->getId() == $entity->getUntranslated()->language()->getId()) {
   +        return AccessResult::forbidden()->cacheUntilEntityChanges($entity);
   

   Do we have test coverage for this?
    

2. +++ b/core/modules/system/src/Tests/Entity/EntityFormTest.php
   @@ -85,12 +99,54 @@ protected function doTestFormCRUD($entity_type) {
   +   * @param string $entity_type_id
   +   *   The ID of entity type to run the tests with.
   ...
   +    $this->drupalPostForm($entity_type_id . '/add', $edit, t('Save'));
   

   The add link for all entity types is not necessarily entity_type_id/add, but this doesn't matter that much since this is just a test helper and it seems to be true at least for the types used in this test. And if that changes then the test would fail explicitly, so it's fine.
    

3. +++ b/core/modules/content_translation/content_translation.module
   @@ -278,8 +281,9 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
   -  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1) {
   +  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1 && ($op == 'edit' || $op == 'default')) {
   

   Minor, but a comment as to why it's only for these operations could be worthwhile.
    

4. I also hesitated that we're adding all this translation-specific stuff to the generic content entity deletion form, but then I read #5 and #7.
    

5. +++ b/core/modules/content_translation/src/Form/ContentTranslationDeleteForm.php
   @@ -7,29 +7,14 @@
   +class ContentTranslationDeleteForm extends ContentEntityDeleteForm {
   
   @@ -41,43 +26,11 @@ public function getFormId() {
   +  public function buildForm(array $form, FormStateInterface $form_state, LanguageInterface $language = NULL) {
   +    if ($language) {
   +      $form_state->set('langcode', $language->getId());
   +    }
   

   At this point is there any reason even to keep this as a separate subclass? Since all the other translation stuff is on the parent and the LanguageInterface parameter is optional.
    

Working on this

On a plane, I'll reply in detail tomorrow.

In the process of fixing #132.1 (great catch), I found a few more glitches and fixed them: now users having edit permissions are no longer able to access the corresponding translation-specific routes. This will allow CT to provide translation-specific operations in #2476563: Entity operations links tied to original entity language, ignore everything else, without cluttering drop-buttons with redundant operations.

Aside from that, I agree that the entity deletion form class hierarchy could use a clean-up. Not sure this is the right issue.

At this point is there any reason even to keep this as a separate subclass? Since all the other translation stuff is on the parent and the LanguageInterface parameter is optional.

The main point of having a separate translation deletion form is providing translators not having entity deletion access to still be able to delete translation. The extended class allows to explicitly set the form language instead of depending on the current one, which may be useful to avoid switching the current language. Another reason is that there may be entity types (such as users) that do not define a deletion form, in which case we still need to delete translations.

Mmh, let's try this

Thanks @plach, #136 definitely helps clarify. I agree that fixing the entity deletion form hierarchy entirely would not make sense in this issue. So instead, let's file a followup to this issue to re-evaluate it? Then we could reference that followup in @todo in the patch, which would help address at least some of my concern. As followup work for this issue it could be prioritized during the beta (but we can discuss it more there).

Just added some debugging code, as I have no failures locally. I'll add the @todo in the next patch.

Created #2491057: Streamline the entity deletion form class hierarchy to address @xjm's concerns.

Bah

Posting the right patch usually helps...

+++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
@@ -267,11 +267,8 @@ protected function doTestTranslationDeletion() {
-    $options = ['absolute' => TRUE];
-    debug(Url::fromRoute('<front>', [], $options)->toString());
-    $url = Url::fromRoute('content_translation.translation_delete_' . $this->entityTypeId, [$this->entityTypeId => $entity->id(), 'language' => 'en'], $options);
-    debug($url->toString());
-    $this->drupalGet($url);
+    $args = [$this->entityTypeId => $entity->id(), 'language' => 'en'];
+    $this->drupalGet(Url::fromRoute('content_translation.translation_delete_' . $this->entityTypeId, $args));

So the ['absolute' => TRUE] was what was causing the test failures? Just to make sure. :)

Nope, I was passing the stringified URL, instead of the URL object...

Ok, pretty sure xjm's feedback has been addressed now, and this has been RTBC long enough now that anyone else who had concerns could've raised them. :)

I re-did my tests from #94 again and can find nothing further to complain about. Great work!! :D

Committed and pushed to 8.0.x. Thanks!

Yay, superb.

Follow-up: #2485499: Allow source translations to be removed