The problem with Drupal 11.3 is that 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active broke the way we "disabled" trash handlers for entity types that are not trash-enabled. Previously, it was enough to remove their service from the container, but now the hook callable is stored in a different place that we can't easily override :/
I've committed a change that determines whether the entity type is trash-enabled rather just looking for a deleted field, fixed this for nodes as well, and removed the code mentioned above because it's useless at this point.
This will be done in 📌 Refactor the way we enable/support simple config and config entities Needs work .
This has been fixed in the 3.0.x branch since we were able to remove the entire workspace association decoration :)
I've opened 📌 [PP-1] Implement workspace conflict validation for config Postponed to add the functionality needed for the remaining @todo's in this MR.
Merged into 3.0.x, thanks!
Turning this issue into a meta for tracking all the elements needed to get wse_config to a stable state.
The problem here was that the system.site config object was ignored by the blanket opt-out of system.* config objects.
Changing that to opt-out specific config objects and a couple of minor changes to the test makes it pass, which proves that simple config is actually supported :)
I've opened 📌 Refactor the way we enable/support simple config and config entities Needs work to fix this in a more general way though.
@traviscarden, do you want to continue working on this test? There are a couple of @todo's left.
There are 3 things discussed in this issue:
1. entity forms throw exceptions on submit for unsupported entity types: this should be fixed in a core issue
2. wse_config doesn't handle enabling support for config entity types without submitting the configuration form: easy to fix in wse_config
3. support for simple config is very much lacking (per #3): opened 📌 Refactor the way we enable/support simple config and config entities Needs work to tackle that
Discussed this a bit in Slack, and the conclusion looks good to me. Nice find :)
@catch, I think it should be safe to exclude all "extra" columns (DefaultTableMapping::getExtraColumns()) since they only repeat the entity data, not the field data we're looking for here.
@danflanagan8,
I wonder if the canonical link for this entity shouldn't activate the preview.
That's an interesting idea, but I think it'd be too risky/disruptive. Most listings (list builders, views) will provide that link by default, which would make accidental workspace preview activation way too easy. Let's provide it as a activate link template instead :)
Postponed on ✨ Allow uses to selectively discard the dependencies Needs review .
Here's the initial attempt at moving the storage doPreSave() override to hook implementations: https://git.drupalcode.org/project/trash/-/merge_requests/9/diffs?commit... At least \Drupal\Tests\trash\Functional\MultilingualNodeTest passed locally :)
While working on this, another question came up: why do we need to loop through and soft-delete all langcodes in \Drupal\trash\TrashStorageTrait::delete() instead of mirroring what \Drupal\Core\Entity\ContentEntityDeleteForm::submitForm() does:
if (!$entity->isDefaultTranslation()) {
$untranslated_entity = $entity->getUntranslated();
$untranslated_entity->removeTranslation($entity->language()->getId());
$untranslated_entity->save();
$form_state->setRedirectUrl($untranslated_entity->toUrl('canonical'));
}
I've worked on the static entity cache issue a bit. Surprisingly, while commenting out the relevant changes from TrashStorageTrait, none of the tests added in this MR were failing (even when running them on 11.1.x). But I was pretty sure the problem is real, so I investigated and fixed it separately in
🐛
Trash doesn't handle the static entity cache properly when switching contexts
Fixed
.
Next, I'll be looking into the pre-save code, which still irks me a bit because the long-term plan is to get as much stuff as possible out of TrashStorageTrait, instead of putting more stuff in :)
This bug was initially found by @twod in ✨ Translation support Needs review , so credited for that :)
An additional commit was needed: https://git.drupalcode.org/project/trash/-/commit/f437502584fd265e6d4972...
I committed this small change to remove some noise from the MR.
Next, I'm going to open the 3.1.x branch because a big feature like this shouldn't be introduced in a patch release, and it would be nice to go through a few alpha/beta/rc releases.
This also provides a nice advantage since we can update the core requirement to >= 11.2.6+ (or even 11.3.0 - we'll see), so we can clean up quite a few things from TrashStorageTrait.
Quoting myself from #38:
I'll check with @plach, but if I remember correctly, it's ok to change multiple translations in a single revision.
I checked an old conversation and I was not remembering correctly, what I said above was wishful thinking, only possible once we have a conflict management solution in core.
@TwoD, right, that makes sense.. even though it's very unfortunate :/
possibly creating more revisions if more than one translation was removed
I'll check with @plach, but if I remember correctly, it's ok to change multiple translations in a single revision.
Posted a suggestion to simplify a bit the code that reorder services.
Optimization: Only check the permissions if the $event->getRequest()->query->has('in_trash') is TRUE.
Good point! Did that and another small optimization so we don't instantiate the entity type manager on most requests.
For the second part, I'm fine with both options. If the string float "hack" works, it's not the worst code needed for Trash to do its job.
@TwoD, thanks for lengthy explanation from #32! It finally made me realize what was bugging me about the latests changes:
The latest version does use
RevisionableStorageInterface::createRevision()where possible, except last in::doPreSave()due to that method always cloning the entity.doPreSave()is expecting the entity to be modified by reference and then passed on to::doSave(). Given we may create multiple new revisions indoPreSave()- something it really wasn't intended to do - we have to tread carefully to make sure we use the correct revision as the source for each language, and that the passed in active translation is handled last and its reference is preserved. Core has already checked that someone isn't trying to delete the default translation and thrown an exception, so that case we don't have to consider. But someone could have called$entity->save()with either the default translation or any other still existing translation active, so we can't make assumptions about that.
First question: why do we need to change things in the (pre)save process, instead of preparing every new translation revision in ::delete(), and saving it as usual?
Second, especially about this part: "But someone could have called $entity->save() with either the default translation or any other still existing translation active"
Why do we care about someone calling $entity->save()? Trash has only been interested in someone calling $entity->delete() until now.
Also, I think the parts where we handle the latest revision static cache are a good candidate for splitting out into their own issue as well, because I'm pretty sure it's a preexisting problem.
I've opened 🐛 Improve handling of the 'in_trash' query parameter Needs review for splitting out these two commits:
https://git.drupalcode.org/project/trash/-/merge_requests/9/diffs?commit...
https://git.drupalcode.org/project/trash/-/merge_requests/9/diffs?commit...
The first one will be reverted because that issues takes a different direction (forbid delete forms if you're not in the active trash context), and the second one should be a clean merge after that MR is committed.
I've tried the approach with a trash/ path prefix instead of the query parameter, but it turned out we'd just bump into the same problems because we need to handle other routes as well, not only view|restore|purge. An example is the Devel route, as can be seen in
🐛
Trash breaks any local tabs on trashed content
Active
.
Ok, I'll rebase the other one on top of the changes made here. Merged into 3.x, thanks!
I had to update this entity (including making it translatable) so it was easier to install and test the trash_test module integration on a working site by adding $settings['extension_discovery_scan_tests'] = TRUE; and installing it with drush - necessary so I could update and regenerate the test views.
Looking at the test changes in the MR, I think the existing test views would work just fine without converting the trash_test entity type to be translatable. There's only one relevant change to a test view config file, and we could keep that but revert the table name changes. That way there would be no conflict with the other issue/MR.
The MR looks great overall! Posted a few comments and questions.
Manual testing would be great, but what's really needed here from my POV is to split up as many things as possible into separate issues in order to get the current monster MR into a reviewable state.
One example is the access handling and the huge amount of test changes. Those could be moved into a smaller issue, and this MR would just add the translation handling.
Another example are the recent changes around ?in_trash=1. I'm not sure if the problems pointed out in #27 are caused by the current MR, but still it would be nice to split out that work.
I tried to review the MR during DrupalCon Vienna, and all the stuff around revisions left me super confused. I'm not sure why we need to do so much special-casing instead of just relying on the fact that the entity objects received by \Drupal\trash\TrashStorageTrait::delete() are already the intended (latest / translation-affected) revisions, and we only need to create a new revision for them using \Drupal\Core\Entity\RevisionableStorageInterface::createRevision(), which already handles all the heavy lifting around translations.
I think it should be easy enough to create a simple default view and a contextual filter for each entity type
Do you mean a view per entity type?
If so, that could potentially blow up the Views UI listing. See the screenshots from 🐛 UX issues in trash, when many entity types are trashable Active for an example of how many entity types could be trash-enabled.
If not, we have the age-old problem: what's the base table for a single view?
Apparently I forgot about this one when I opened 📌 Move the remaining UI bits to the workspaces_ui module Active a year later.
Yup, the MR is still work-in-progress, but I set it to NR for architectural (or any kind of) review.
I'm pretty sure this is still a problem, I'm not aware of any changes to that code since this issue was opened :)
But \Drupal\workspaces\WorkspaceManager::purgeDeletedWorkspacesBatch() expects to only receive one revision ID per entity ID and now it receives multiple.
I'm not sure I understand this part of the issue summary. \Drupal\workspaces\WorkspaceManager::purgeDeletedWorkspacesBatch() doesn't expect that, it should purge all revisions tracked by a workspace.
The only part that could be problematic is if \Drupal\wse\WseWorkspaceAssociation::getAssociatedInitialRevisions() returns multiple revision IDs for the same entity, but that means there's a data consistency problem in the sites's database.
@kristiaanvandeneynde, could you provide more details on what was the different output you noticed between core's and WSE's implementation of the two methods?
These overrides are still useful for users of the workspaces_parallel module, where entities can be edited in Live while they're tracked by a workspace. Reverted the MR for now, and will provide a proper fix instead.
Pushed the WIP code. The provider class is a bit messy, but it should be cleaned up when we get all functional tests passing.
Here it is :) 📌 Implement dependency tracking for moderated content using workspaces Active
Reviewed the MR and it looks good to me, the changes are now cleanly scoped to use a different field type when we're not specifically testing text_with_summary..
Thanks! Updated and published the CR: https://www.drupal.org/node/3551450 →
Following the steps to reproduce from #4 I am able to edit the node (step 5).
@the_g_bomb, do you have any other modules installed that might interfere with revisions?
Thanks for pointing this out! We no longer need to add that library at all since Trash is using the new Icon API to add its icon.
amateescu → made their first commit to this issue’s fork.
I think points 2. and 4. from the API changes part of the IS are no longer accurate and should be removed.
Read this through a few times and I like that it could be simplified so much for the initial functionality! Posted a few minor suggestions that can be applied if deemed useful, otherwise I think it's ready :)
I can cover Workflows. I'm already a topic maintainer for a few other subsystems :)
This is ready now. The MR is easier to review locally with git diff 11.x --color-words.
Posted a couple of comments. And the test failures seem related :)
Started a MR with an initial proposal. I don't love it but it's the best I could come up with :)
I think RTBC is the correct status when we have the backport MR ready.
Here it is: 📌 Optimize database writes when re-saving a pending revision as the default one Active
@smustgrave, yup, and the test for it is in ::testMoveAllTrackedEntities() :)