Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete content node along with its media and file #905

Merged
merged 12 commits into from
Dec 6, 2022

Conversation

shriram10567
Copy link

@shriram10567 shriram10567 commented Oct 27, 2022

GitHub Issue: Deleting content node does not delete media/files associated with node

What does this Pull Request do?

This PR adds a checkbox for deleting media in the delete form of islandora objects and lists all the media associated with the object in the form of list

What's new?

  • Does this change add any new dependencies? no
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? no
  • Could this change impact execution of existing code? no

How should this be tested?

Create some content of the Islandora repository item type and add media to that content using media of field. click the content you created from the content page. In the available tabs for the content, click 'Delete' to see the changes.

the feature can be turned on and off with a checkbox in islandora settings page.

Documentation Status

  • Does this change existing behaviour that's currently documented? no
  • Does this change require new pages or sections of documentation? probably, yes
  • Who does this need to be documented for?
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Any additional information that you think would be helpful when reviewing this
PR.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

@shriram10567 shriram10567 changed the title Dev Deleting content node does not delete media/files associated with node Oct 27, 2022
@shriram10567 shriram10567 changed the title Deleting content node does not delete media/files associated with node Delete content node along with its media Nov 1, 2022
islandora.module Outdated
];

$medias = $utils->getMedia($form_state->getFormObject()->getEntity());
$media_list = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

Just a note, rendering markup directly within Drupal is normally frowned upon. That is if anything else later on wanted to modify what was here the markup is already generated and you'd have to resort to hacking on strings.

Alternatively if you utilize the #theme key to call item_list you can achieve the same thing while still allowing the theme layer/other modules et al to hook in. More information can be found in the docs as well as an example can be found here.

@alxp
Copy link

alxp commented Nov 2, 2022

Hi @shriram1056 ,

In the committers' call today it was brought up that you will need to explicitly delete files along with media if there are zero other references to the file. Drupal will only automatically delete it if it is set to temporary, but this is not the case after it's been created and associated with another entity.

This should be a straightforward addition.

@shriram10567 shriram10567 changed the title Delete content node along with its media Delete content node along with its media and file Nov 8, 2022
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated
Comment on lines 336 to 342
$form_object = $form_state->getFormObject();
$utils = \Drupal::service('islandora.utils');

if ($form_object instanceof EntityForm) {
$entity = $form_object->getEntity();

if ($entity->getEntityTypeId() == "node" && $utils->isIslandoraType($entity->getEntityTypeId(), $entity->bundle()) && strpos($form['#form_id'], 'delete_form') !== FALSE) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is specifically targeting node, and furthermore, the EntityForm of it, should this instead be more targetedly adjusting the definition of node content entities, such as substituting our own delete and delete-multiple-confirm forms?

In any case, I almost feel like this behaviour probably should be behind a feature toggle, or in another (sub)module, as something like the entity_reference_integrity module with its entity_reference_integrity_enforce submodule should prevent this functionality from working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 5c24c19 a toggle was added.

Copy link
Member

@jordandukart jordandukart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments after doing a bit of a in-depth dive. If you want some clarification in a more real-time fashion feel free to ping me in the Islandora Slack.

config/install/islandora.settings.yml Show resolved Hide resolved
islandora.libraries.yml Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
src/Form/IslandoraSettingsForm.php Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
islandora.module Outdated Show resolved Hide resolved
@jordandukart
Copy link
Member

Will let this sit for another 24 hours in case someone else wants to jump in but if nothing comes up will press the button.

@jordandukart jordandukart merged commit 023b24b into Islandora:2.x Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants