Problem/Motivation

The Trash module currently exists in contrib, as part of the Workflow Initiative we hope to move it into core.

Proposed resolution

- Port Trash module to depend on the archived state in Content Moderation rather then Multiversion
- Update Trash module to be an experimental core module

Remaining tasks

Postponed on #3088643: Mark Workspaces as a stable core module

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#49 2797247-49.patch42.79 KBravi.shankar
#29 interdiff.txt25.51 KBtimmillwood
#29 add_experimental_trash-2797247-28.patch42.73 KBtimmillwood
#26 interdiff-2797247-25-26.txt617 bytesnaveenvalecha
#26 2797247-26.patch33.64 KBnaveenvalecha
#25 interdiff-2797247-22-25.txt3.46 KBnaveenvalecha
#25 2797247-25.patch33.6 KBnaveenvalecha
#24 2797247-22.patch33.63 KBnaveenvalecha
#22 interdiff-2797247-21-22.txt13.7 KBnaveenvalecha
#22 2797247-22.patch44.47 KBnaveenvalecha
#21 interdiff.txt11.09 KBtimmillwood
#21 add_experimental_trash-2797247-21.patch31.09 KBtimmillwood
#19 interdiff.txt17.35 KBtimmillwood
#19 add_experimental_trash-2797247-19.patch29.67 KBtimmillwood
#18 interdiff.txt10.83 KBtimmillwood
#18 add_experimental_trash-2797247-18.patch26.75 KBtimmillwood
#17 interdiff.txt5.03 KBtimmillwood
#17 add_experimental_trash-2797247-17.patch32.52 KBtimmillwood
#16 interdiff.txt5.15 KBtimmillwood
#16 add_experimental_trash-2797247-16.patch29.52 KBtimmillwood
#13 interdiff-2797247-9-13.txt4.86 KBnaveenvalecha
#13 2797247-13.patch28.63 KBnaveenvalecha
#12 Screenshot from 2016-09-09 08-37-38.png28.01 KBtimmillwood
#10 interdiff-2797247-7-9.txt11.11 KBnaveenvalecha
#10 2797247-9.patch28.71 KBnaveenvalecha
#6 interdiff-2797247-5-7.txt897 bytesnaveenvalecha
#6 2797247-7.patch26.19 KBnaveenvalecha
#5 interdiff-2797247-24.txt14.92 KBnaveenvalecha
#5 2797247-4.patch25.31 KBnaveenvalecha
#2 add_experimental_trash-2797247-2.patch25.01 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
25.01 KB

Initial patch based on 8.x-2.x of the Trash contrib module.

dixon_’s picture

Status: Needs review » Needs work

The last submitted patch, 2: add_experimental_trash-2797247-2.patch, failed testing.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
25.31 KB
14.92 KB

1. Fixed coding standards at couple of places.
2. Removed the references of some of the multiversion module

Working on the hook_help of the module to get these failures fixed https://www.drupal.org/pift-ci-job/452161

naveenvalecha’s picture

This will fix the failures in #2
Will work on the tests tomorrow, if anyone wants to beat me and write the tests first then go ahead

naveenvalecha’s picture

Issue summary: View changes

The last submitted patch, 5: 2797247-4.patch, failed testing.

naveenvalecha’s picture

Issue summary: View changes
naveenvalecha’s picture

dawehner’s picture

It is quite nice how small this module/feature actually is!

  1. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,250 @@
    +
    +    foreach ($this->getModeratedEntityTypes() as $entity_type_id => $entity_type) {
    +      $entities = $this->loadEntities($entity_type_id);
    +      $items[$entity_type_id] = [
    +        '#type' => 'link',
    +        '#title' => $entity_type->get('label') . ' (' . count($entities) . ')',
    +        '#url' => Url::fromRoute('trash.entity_list', ['entity_type_id' => $entity_type->id()]),
    +      ];
    +    }
    +    return [
    +      '#theme' => 'item_list',
    +      '#items' => $items,
    +      '#title' => $this->t('Trash bins'),
    +    ];
    

    I'd have expected this list to have a pager somehow

  2. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,250 @@
    +  protected function loadEntities($entity_type_id = NULL) {
    

    Nitpick: What about using loadArchivedEntities() to make it clearer what this method is trying to achieve?

  3. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,107 @@
    +  public function entityDelete() {
    ...
    +      $content_moderation_state->moderation_state->target_id = 'archived';
    +      if ($content_moderation_state->save()) {
    
    +++ b/core/modules/trash/src/Routing/TrashRouteSubscriber.php
    @@ -0,0 +1,74 @@
    +      $route = $collection->get("entity.$entity_type_id.delete_form");
    +      if (!empty($route)) {
    +        $defaults = $route->getDefaults();
    +        unset($defaults['_entity_form']);
    +        $defaults['_controller'] = '\Drupal\trash\Controller\TrashDeleteController::entityDelete';
    +        $route->setDefaults($defaults);
    +        // TODO: Get the right permission.
    +        $route->setRequirements(['_permission' => 'access unpublished content']);
    

    This IMHO needs CRSF protection, doesn't it?

  4. +++ b/core/modules/trash/trash.routing.yml
    @@ -0,0 +1,39 @@
    +  path: '/admin/trash'
    

    I'm wondering whether this conceptually belongs below /admin/content

timmillwood’s picture

1) not sure this needs a pager as it's just a summary page showing how many trashed entities per entity type.

2) ok

3) I guess, I will work out if or how this is possible.

4) /admin/content is for nodes, trash is for any moderated entity types, in core this is nodes and blocks. Will see what UX people think.

naveenvalecha’s picture

FileSize
28.63 KB
4.86 KB

Addressed #11.2

As per chat with tim over IRC we are fine to add the pager of 5 or 10 as per #11.1

#11.1, #11.3 will work on it.

dawehner’s picture

1) not sure this needs a pager as it's just a summary page showing how many trashed entities per entity type.

Ha, I now understand what confused me ... why do you not use an actual entity count query? Instead of loading entities and then just counting them

3) I guess, I will work out if or how this is possible.

user.logout.http:
  defaults:
    _controller: \Drupal\user\Controller\UserAuthenticationController::logout
  requirements:
    _csrf_token: 'TRUE'

Here is an example.

catch’s picture

Tagging.

timmillwood’s picture

* Added _csrf_token: 'TRUE' to delete route
* Added entity parameter to delete route
* Checked entity is moderated
* Return delete form is entity is not moderated
* Update moderation_state if entity is moderated

timmillwood’s picture

Adding an initial test for testing deleting moderated nodes.

timmillwood’s picture

Reworked the trash page to show all entity types, and fixed the test.

timmillwood’s picture

Added test for blocks.

Wim Leers’s picture

  1. +++ b/core/MAINTAINERS.txt
    @@ -471,6 +471,9 @@ Tour module
    +Trash module
    +- Tim Millwood 'timmillwood' https://www.drupal.org/u/timmillwood
    

    :)

  2. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +   * The database;
    

    Nit: s/;/./

  3. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +   * @var \Drupal\Core\Datetime\DateFormatter
    ...
    +   * @param \Drupal\Core\Datetime\DateFormatter $date_formatter
    ...
    +  public function __construct(Connection $database, DateFormatter $date_formatter, ModerationInformationInterface $moderation_information) {
    

    Nit: Must be typehinted to the interface.

  4. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +   *   A render array as expected by drupal_render().
    ...
    +   *   A render array as expected by drupal_render().
    

    Just A render array. is better. Because drupal_render() is deprecated.

  5. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +            'url' => Url::fromRoute('restore.form', [
    +              'entity_type_id' => $entity->getEntityTypeId(),
    +              'entity_id' => $entity->id(),
    +            ]),
    ...
    +            'url' => Url::fromRoute('purge.form', [
    +              'entity_type_id' => $entity->getEntityTypeId(),
    +              'entity_id' => $entity->id(),
    +            ]),
    

    The route parameters are identical. Let's create this once and use it twice?

  6. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +            '#access' => $entity->access('view'),
    

    You should be specifying $return_as_object = TRUE.

    (Yes, I know, the DX of this sucks. The reason: BC.)

    Even if you can get away without this, new code should be setting the right example.

  7. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +      '#empty' => $this->t('The trash is empty.'),
    

    This is a strange sentence. What about There is no trash. or The trash bin is empty.?

    Perhaps it's just my non-native speaker ears that this sounds strange to.

  8. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +   * @return \Drupal\Core\Entity\EntityInterface[]
    +   *   An array of entity objects indexed by their IDs. Returns an empty array
    +   *   if no matching entities are found.
    ...
    +   * @return \Drupal\Core\Entity\EntityInterface[]
    +   *   An array of entity objects indexed by their IDs. Returns an empty array
    +   *   if no matching entities are found.
    

    Will this not also be ContentEntityInterface entities?

    Let's be consistent, I'd say.

  9. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +    $query = $this->database
    +      ->select($data_table, 'data_table')
    +      ->fields('data_table', ['content_entity_type_id', 'content_entity_id'])
    +      ->condition('moderation_state', 'archived');
    +
    +    $entity_data = $query->execute()->fetchAllAssoc('content_entity_id', \PDO::FETCH_ASSOC);
    

    Woah, explicit queries. Not seen that in a while. I'm assuming there's no other way?

  10. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +   * Returns the header array for entity type.
    

    s/header array/table header render array/

  11. +++ b/core/modules/trash/src/Controller/TrashController.php
    @@ -0,0 +1,182 @@
    +  /**
    +   * Returns the list of Moderated entity types.
    +   *
    +   * @return \Drupal\Core\Entity\EntityInterface[]
    +   *   An array of entity objects indexed by their IDs. Returns an empty array
    +   *   if no matching entities are found.
    +   */
    +  protected function getModeratedEntityTypes() {
    +    $entity_types = $this->entityTypeManager()->getDefinitions();
    +    return array_filter($entity_types, function (EntityTypeInterface $entity_type) use ($entity_types) {
    +      return $this->moderationInformation->canModerateEntitiesOfEntityType($entity_type);
    +    });
    +  }
    

    Unused method.

  12. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,126 @@
    +   *   Returns the redirect url object.
    ...
    +   * Returns the url object for redirect path.
    ...
    +   *   The entity for which we want the redirect url.
    ...
    +   *   The redirect url object.
    

    Nit: s/url/URL/

  13. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,126 @@
    +    $parameters = $this->routeMatch->getParameters();
    ...
    +    $entity = current(reset($parameters));
    

    Hm… is this always going to be the first parameter? This seems brittle.

  14. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,126 @@
    +    /** @var ContentEntityInterface $entity */
    

    Nit: needs FQCN.

  15. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,126 @@
    +    $entity->moderation_state->target_id = 'archived';
    

    Why 'archived'? I'm missing documentation.

    Also, this then probably should be a class constant. Not on this class probably though.

  16. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,126 @@
    +      drupal_set_message(t('The @entity %label has been moved to the trash.', [
    

    Nit: s/t()/$this->t()/

  17. +++ b/core/modules/trash/src/Controller/TrashDeleteController.php
    @@ -0,0 +1,126 @@
    +    return $this->redirect($this->getRedirectUrl($entity)
    +      ->getRouteName(), $this->getRedirectUrl($entity)->getRouteParameters());
    

    This is some rather stange formatting.

  18. +++ b/core/modules/trash/src/Form/PurgeForm.php
    @@ -0,0 +1,116 @@
    +    return $this->t('Are you sure you want to purge “@label”?', ['@label' => $this->entity->label()]);
    ...
    +    return $this->t('Purging “@label” from the database should only be done as a last resort when sensitive information has been introduced inadvertently into a database. In clustered or replicated environments it is very difficult to guarantee that a particular purged document has been removed from all replicas.', ['@label' => $this->entity->label()]);
    

    Should this not be using %label rather than "@label", for consistency with the rest of core?

  19. +++ b/core/modules/trash/src/Form/PurgeForm.php
    @@ -0,0 +1,116 @@
    +      throw new NotFoundHttpException(t('Deleted entity with ID @id was not found.', ['@id' => $entity_id]));
    ...
    +    drupal_set_message(t('The @entity “@label” has been purged.', [
    

    s/t()/$this->t()/

  20. +++ b/core/modules/trash/tests/src/Functional/AdminContentDeleteTest.php
    @@ -0,0 +1,129 @@
    +    $this->permissions = array_merge($this->permissions,
    +      [
    +      'administer nodes',
    

    Supernit: Move the [ to the end of the first line.

  21. +++ b/core/modules/trash/trash.routing.yml
    @@ -0,0 +1,29 @@
    +    _title: 'Purge'
    

    s/Purge/Restore/ :)

  22. +++ b/core/modules/trash/trash.services.yml
    @@ -0,0 +1,6 @@
    +      - { name: event_subscriber, priority: -100 }
    

    Needs documentation for the priority. i.e. why -100?

timmillwood’s picture

Adding restore tests
Pulling changes from Trash 8.x-2.x contrib

Not addressed anything from #20 yet (Thanks @Wim Leers)

naveenvalecha’s picture

Addressed Wim feedback.

Status: Needs review » Needs work

The last submitted patch, 22: 2797247-22.patch, failed testing.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
33.63 KB

Wrong patch in #22 :(
Edit :
1. :)
2. Addressed
3. Done
4. Taken care and also noted.
5. Done
6. Ok
7. Done
8. Done
9. yup that' was the only way. Also discussed the same with Daniel yesterday.
10. Done
11. Removed.
12. Done
13. Pending
14. Done
15. yup the UX team is working on the mockups but eventually we should have a setting to choose the archive state and restore state, the UX team want this to be a site wide setting, and per bundle setting (like other content_moderation settings.
16. Done
17. Filed an upstream issue https://www.drupal.org/node/2799875 will discuss and get this fixed
18. Great find. Done. yup exactly there's no need to sanitize it as its not user provided.
19. Done
20. Done
21. Done
22. Pending

naveenvalecha’s picture

naveenvalecha’s picture

The last submitted patch, 24: 2797247-22.patch, failed testing.

The last submitted patch, 25: 2797247-25.patch, failed testing.

timmillwood’s picture

Pulling updates from contrib, interdiff with #21.

timmillwood’s picture

Status: Needs review » Needs work
timmillwood’s picture

Today I have been working on the Trash module.

I was in the mindset that we were waiting on Workflows to land to get trash in, but as I worked though it today I came face to face with more questions than I was expecting. This lead me down the path of wondering if we need Workflows at all for Trash. It also seemed to look back to some of the discussions we had some time ago about the status field in core.

It all started after implementing the Trash Workflow config entity. It had two states "Trashed" and "Not in trash", then two transitions to switch between the two states. Some of the logic which was in Trash when it depended on Content Moderation was removed, and replaced with simple code that if an entity type extends EntityPublishedInterface it can be trashed. This is because something that is trashed needs to be unpublished, however something that is not in the trash could be either published or unpublished.

Next the Trash WorkflowType plugin was implemented, and all was going well until it was time to store the state somewhere. This couldn't be done with ContentModerationState entity type, because this is part of Content Moderation module. Do we need a TrashState entity type? surely that's overkill? So what are we looking to store? If something is in the trash or not. Just a boolean value! So why do we need Workflows?

If we were doing this in contrib (as we already have) we'd just add a base field on all entity types we want to trash. We can't do this in core as it would mean the module is uninstallable. So where can we store it? in key_value? in a custom table? in a new entity type?

I think Views will help us determine the answer. Most integrations with Trash will come via Views. If we can a consolidated list of Trashed entities it will need to be a View of something like TrashState entity type, with a reference to the trashed entity.

Gábor Hojtsy’s picture

@timmillwood I personally think that entities could have a supporting field for trash module even if trash module is not installed? Entities have a promoted to front page field even though there is nothing about them that makes them appear on the front page without views module anymore :) There are a whole bunch of translation supporting fields even though translation may not be used, etc. The key question is how core is this functionality.

timmillwood’s picture

@Gábor Hojtsy - That's one possible idea.

As I'm discovering in #2820848: Make BlockContent entities publishable adding a field to entity types isn't trivial, but it's definitely a possibility.

amateescu’s picture

Next the Trash WorkflowType plugin was implemented, and all was going well until it was time to store the state somewhere. This couldn't be done with ContentModerationState entity type, because this is part of Content Moderation module. Do we need a TrashState entity type?

I thought the plan was to use Content Moderation for Trash, not Workflows directly :)

Gábor Hojtsy’s picture

ATM a content type can only have one workflow, so the simplest implementation would be to have an "In trash" or "Deleted" state in the regular publishing workflow, howeve the default UI would be confusing with a button to delete it alongside the other draft/publish buttons.

naveenvalecha’s picture

#35
+1
Tim,
Could you summarize the plan here what are the next action items to push this forward?

//Naveen

timmillwood’s picture

So I think the plan is to allow content moderation workflows on all revisionable entity types, and multiple content moderation workflows per entity type / bundle. Then rewrite trash to depend on Content Moderation.

#2799785: Entity types with non-config bundles can not be moderated is to get moderation of all revisionable entity types.
#2835545: Provide a Workflow FieldType that references a workflow entity and tracks the current state of an entity is to get multiple workflows per entity type.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ptmkenny’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cola’s picture

Any updates on this issue?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

andypost’s picture

Issue tags: +Needs reroll
ravi.shankar’s picture

Rerolled patch #29.

amateescu’s picture

Assigned: timmillwood » Unassigned
Status: Needs work » Active

IMO this needs an entirely new architectural approach, there's no point in rerolling the existing patches.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Issue summary: View changes
Status: Active » Postponed
Related issues: +#3088643: Mark Workspaces as a stable core module

Discussed with the release managers and based on that last comment marking this postponed on workspaces becoming stable.

lauriii’s picture

Would it be possible to document why the decision was made to postpone this on the Workspaces? This module doesn't directly depend on Workspaces so it isn't self-explanatory why we would make it a hard blocker for this issue.

catch’s picture

@lauriii we shouldn't add a new experimental module that is dealing with deep entity system architecture while we have one already in core that's not stable yet. And more specifically, the time spent working on trash module instead of resolving fundamental blockers to workspaces being is part of the reasons it didn't reach stable 2-3 years ago.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.