Can't delete the translation of a media

Issue is reproducible as per above steps. Looking into it to provide a fix

Looks like the retrieval of deleted entity to show deletion details is being done after deletion is done. Moved the deletion data retrieval before deleting the entity itself.

Patch attached to be re rolled into 8.5.x and 8.6.x as I can see the same code in 8.6.x too.

I am having the same Issue using translated Media entities, the Patch works for me.

Re-rolled for 8.7.x-dev
and same patch can be applied for 8.6.x-dev

Will test patch for 8.7.x and 8.6.x and update

This sounds like a serious bug, could you also provide a test and a fail patch for this?

@seanB The patch submitted in #9 was wrong because it is not updated code. I have provided the new patch with updated code for 8.7.x and also 8.6.x versions.

Note that this issue however was raised originally for 8.5.x hence need to push the fixes for all 3 versions of the core.

I would recommend testing the patches for 8.6.x and 8.7.x since I could not test in both versions due to issue #2840218: Installer sometimes gives "Drupal already installed" message even if install failed

Wrong file name. Updated now.

@seanb The tests have passed for all version. Kindly add these into respective versions and close this issue if all is good. Let me know if you need anything else.

@kswamy, thanks for the patches. Could you write a new test to prove that the change actually fixed the issue?

When we have this test, we should have a patch with the test only, that should fail. Then we should have a second patch containing the test and the change in core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php to prove the change actually fixed the issue.

Also, if this is a media specific issue, changing the generic ContentEntityDeleteForm does not sound like a good idea.

Looks like the retrieval of deleted entity to show deletion details is being done after deletion is done. Moved the deletion data retrieval before deleting the entity itself.

Do you know where exactly we are trying to get data for the deleted entity? Because that is probably the place that needs fixing.

@seanB Sure. I will backtrack and try to find out if we can provide a fix at media specific level as this issue is not there for normal entity delete operation.

Since based on above finding the tests would vary, I will first work on finding if this indeed needs change in media specific file.

@seanB Below are my findings.

1. This issue does not occur if we translate a page and delete the translation for that page.
2. So I tried to find out if the flow of control is different in the case of deletion of translation for page.
3. Looks like getDeletionMessage function in ContentEntityDeleteForm.php is not getting called at all in case of a page even though for both the below code is executed which is just above the call for getDeletionMessage

    if (!$entity->isDefaultTranslation()) {
      $untranslated_entity = $entity->getUntranslated();
      $untranslated_entity->removeTranslation($entity->language()->getId());
      $untranslated_entity->save();
      $form_state->setRedirectUrl($untranslated_entity->urlInfo('canonical'));
    }

4. I believe the form state is different in both cases which causes it to work for page content type whereas for media content type it fails.
And moreover though this file is named as ContentEntityDeleteForm.php, the content looks like it is specific to deletion of translation and not common for all contents. You can see this if you go thorough the entire file that we find only translation related code. So I believe changing in this file will change only for content translations delete.

Let me know if we can go ahead with above patches, in which case I will proceed with writing tests.

@seanB Looks like the delete and multiple delete of media entity type alone is performed in Drupal\Core\Entity\ContentEntityDeleteForm and Drupal\Core\Entity\Form\DeleteMultipleForm rather than in Drupal\media\MediaForm as given below in Drupal\media\Entity\Media in comments.

 *     "form" = {
 *       "default" = "Drupal\media\MediaForm",
 *       "add" = "Drupal\media\MediaForm",
 *       "edit" = "Drupal\media\MediaForm",
 *       "delete" = "Drupal\Core\Entity\ContentEntityDeleteForm",
 *       "delete-multiple-confirm" = "Drupal\Core\Entity\Form\DeleteMultipleForm",
 *     },

Hence looks like the placement of fix appears to be correct. Working on adding tests. Looks like Drupal\Tests\media\Kernel\MediaTranslationTest would be the best place to add a delete test case since there seems to be function for only storage and retrieval.

Found the issue. Apparently this->get('name') can not be called on a deleted entity. If we change this to $this->getEntityKey('label') it is no problem?!

Attached is a patch to fix this. Implemented a media version of ContentTranslationUITestBase to test this while I was at it. This proves our fix and adds much needed test coverage.

Fail patch failed.

...and I meant to change the status. Silly me.

Thanks @seanB. I will test this and update.

Not sure why this is being triggered just for media and not for other entities, but I think the proper fix would be to retrieve the message before deleting the entity/translation in ContentEntityDeleteForm::submitForm(). Let's just put it in a local variable together with a comment along the lines of "Retrieve the deletion message before perfoming the actual deletion as it may rely on information provided by the entity."

That said using getEntityKey() is actually a small performance improvement, so might make sense, as well, but I don't think it's a sufficient fix. For example of - for whatever reason - the label is empty and needs to be retrieved from the media source, that might not actually work after the media item is deleted. It might rely on a resource that was deleted along with the media item, for example.

@tstoeckler So, do you confirm that the patch I provided in comment https://www.drupal.org/project/drupal/issues/2986898#comment-12705331 is correct? I have done the same except have not added comments.
So we might need to combine my patch along with @seanB changes which would give the sufficient fix and also improve the performance. Do you suggest the same?

Re #28: Ahh I missed #14, thanks for pointing that out. However, we should not be calling the messenger before the actual deletion happened. We should just be fetching the message up-front. In other words:

$message = $this->getDeletionMessage();

// Actual deletion.

$this->messenger()->addStatus($message);

In #14, if something goes wrong with the deletion, we would still be showing the success message, even though it might not actually have been successful.

@tstoeckler Ok. Got it. I will do one thing. Will update the patch #2986898-14: Can't delete the translation of a media as you told and also add the changes made by @seanB in #2986898-22: Can't delete the translation of a media and create a new patch with the test also so that we can see if it passes. Then I believe we should be ready to merge the patch.

That sounds like a plan, thanks! 😉👍🏿

Not sure why this is being triggered just for media and not for other entities

@tstoeckler the problem is in the custom getName() methog the media module uses from the label() method. The label method in media calls Media::getName(). The Media::getName() method uses $this->get('name'); instead of
$this->getEntityKey('label'); which is the default in ContentEntityBase::label().

So the actual bug is in the media module. There is no need to put the message in a variable since ContentEntityDeleteForm has a copy of the entity on which you should be able to call the label() method without any issues.

@seanB The patch in #22 applies clean and fixes the issue. However have a comment.

Considering the changes done, it would be appropriate to rename the getName() function of the Media module to getLabel() as it actually fetches the label.

Rest of the changes looks fine if we are planning to proceed forward with this patch.

Since getName() is on the interface we can not do that because of BC. The label key is also 'name', so we deliberately chose to add the getName() method.

Thanks for the response @seanB. I'm not sure I agree, though.

ContentEntityDeleteForm has a copy of the entity on which you should be able to call the label() method without any issues.

I'm not sure that's true. It does keep an instance of the entity in memory, but I'm not sure it's safe to call label() on it. In other words, I think Media has exposed a legitimate bug here. I haven't dug through this, so this is just base on assumptions, and I totally might be worng, but...

...as far as I can tell calling label() after the entity has already been deleted only works be cause the static cache in $this->entityKeys (or, to be precise in $this->translatableEntityKeys in this case) has already been populated for the label, so that it does not need to be fetched. But although it's very likely that at some point during the request the label of the entity being deleted has already been fetched - concretely when building the "Are you sure you want to delete %label" question - and, thus, it's very likely that the approach in #22 would always work, I still don't think it's actually correct to make this assumption. If the entity is reloaded for some reasons and the cache is not populated, or if someone outputs a different question that does not include the entity label and the label does not end up being called prior to the deletion, then #22 would all of a sudden still result in the same failure.

Again, it's totally possible that I'm missing something, but I'm not quite convinced that ##2 isn't the way to go here. Thanks for taking the time!

If we want to be safe we can do both. I'm not totally against fetching the message before delete. The actual issue "Can't delete translation of media" is solved by fixing the getName() method though.

If we want to improve the way we fetch the message in ContentEntityDeleteForm that is fine as well, but maybe that is better solved in a separate issue?

OK sure, that's fine. I don't have an opinion either way of issue scoping.

@seanB Ok. Makes sense. Thanks. Let me know if I need to do anything else here.

@kswamy could you roll a patch that fetches the message before the actual delete and incorporate the media fix/test from #22?

@seanB sure. Will do

@seanB Rolled patch with the updated suggested by @tstoeckler along with the fix given in #22 with the test.

I can't RTBC this but it looks good to me. Thanks!

Updated status ....oops...I mean assignee

@tstoeckler could you review this please?

Yup, looks good to me, as well. Thanks!

@seanB Just want to check if any plans to commit this to the Drupal 8.7 DEV core ?

@kswamy we have to wait until one of the core committers has time to take a look. I can only sign off from a maintainers perspective.

@seanB Ok. Thank You.

Committed and pushed 4cafe992ac to 8.7.x and 547bcff648 to 8.6.x. Thanks!

Hi,

I am still getting the same issue in 8.6.4. Issue is only fixed after reinstalling Media module.