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
Comment | File | Size | Author |
---|---|---|---|
#49 | 2797247-49.patch | 42.79 KB | ravi.shankar |
#29 | interdiff.txt | 25.51 KB | timmillwood |
#29 | add_experimental_trash-2797247-28.patch | 42.73 KB | timmillwood |
Comments
Comment #2
timmillwoodInitial patch based on 8.x-2.x of the Trash contrib module.
Comment #3
dixon_Comment #5
naveenvalecha1. 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
Comment #6
naveenvalechaThis 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
Comment #7
naveenvalechaComment #9
naveenvalechaComment #10
naveenvalechaComment #11
dawehnerIt is quite nice how small this module/feature actually is!
I'd have expected this list to have a pager somehow
Nitpick: What about using loadArchivedEntities() to make it clearer what this method is trying to achieve?
This IMHO needs CRSF protection, doesn't it?
I'm wondering whether this conceptually belongs below
/admin/content
Comment #12
timmillwood1) 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.Comment #13
naveenvalechaAddressed #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.
Comment #14
dawehnerHa, 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
Here is an example.
Comment #15
catchTagging.
Comment #16
timmillwood* 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
Comment #17
timmillwoodAdding an initial test for testing deleting moderated nodes.
Comment #18
timmillwoodReworked the trash page to show all entity types, and fixed the test.
Comment #19
timmillwoodAdded test for blocks.
Comment #20
Wim Leers:)
Nit: s/;/./
Nit: Must be typehinted to the interface.
Just
A render array.
is better. Becausedrupal_render()
is deprecated.The route parameters are identical. Let's create this once and use it twice?
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.
This is a strange sentence. What about
or ?Perhaps it's just my non-native speaker ears that this sounds strange to.
Will this not also be
ContentEntityInterface
entities?Let's be consistent, I'd say.
Woah, explicit queries. Not seen that in a while. I'm assuming there's no other way?
s/header array/table header render array/
Unused method.
Nit: s/url/URL/
Hm… is this always going to be the first parameter? This seems brittle.
Nit: needs FQCN.
Why
'archived'
? I'm missing documentation.Also, this then probably should be a class constant. Not on this class probably though.
Nit: s/t()/$this->t()/
This is some rather stange formatting.
Should this not be using
%label
rather than"@label"
, for consistency with the rest of core?s/t()/$this->t()/
Supernit: Move the
[
to the end of the first line.s/Purge/Restore/ :)
Needs documentation for the priority. i.e.
Comment #21
timmillwoodAdding restore tests
Pulling changes from Trash 8.x-2.x contrib
Not addressed anything from #20 yet (Thanks @Wim Leers)
Comment #22
naveenvalechaAddressed Wim feedback.
Comment #24
naveenvalechaWrong 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
Comment #25
naveenvalechaComment #26
naveenvalechaComment #29
timmillwoodPulling updates from contrib, interdiff with #21.
Comment #30
timmillwoodThis will need a rewrite once workflows are in.
#2779647: Add a workflow component, ui module, and implement it in content moderation
Comment #31
timmillwoodToday 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.
Comment #32
Gábor Hojtsy@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.
Comment #33
timmillwood@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.
Comment #34
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI thought the plan was to use Content Moderation for Trash, not Workflows directly :)
Comment #35
Gábor HojtsyATM 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.
Comment #36
naveenvalecha#35
+1
Tim,
Could you summarize the plan here what are the next action items to push this forward?
//Naveen
Comment #37
timmillwoodSo 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.
Comment #40
ptmkenny CreditAttribution: ptmkenny commentedComment #44
cola CreditAttribution: cola commentedAny updates on this issue?
Comment #48
andypostComment #49
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedRerolled patch #29.
Comment #50
amateescu CreditAttribution: amateescu as a volunteer commentedIMO this needs an entirely new architectural approach, there's no point in rerolling the existing patches.
Comment #55
quietone CreditAttribution: quietone at PreviousNext commentedDiscussed with the release managers and based on that last comment marking this postponed on workspaces becoming stable.
Comment #56
lauriiiWould 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.
Comment #57
catch@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.