Account created on 21 February 2010, about 15 years ago
#

Merge Requests

More

Recent comments

🇷🇴Romania amateescu

Merged into 3.x. Thanks @plach for reviewing this MR!

🇷🇴Romania amateescu

Opened a followup for #19 and added a @todo pointing to it :)

🇷🇴Romania amateescu

Thanks @krisahil, we're almost there! Added a comment to the MR, the data provider is not necessary and should be removed :)

🇷🇴Romania amateescu

I've been thinking about this problem lately and I think we should add a Compact overview setting that displays the list of trash-enabled entity types in a select element and hides the second-level local tasks.

Let me know if you can test this a bit and what do you think about it :)

🇷🇴Romania amateescu

In that case, some debugging is needed. First, I would look at \Drupal\trash\TrashStorageTrait::delete() and check if it's immediately going to the parent delete method, which means the trash context is not active. If that's the case, the next step is to put a breakpoint in TrashManager::setTrashContext() and see who's changing the trash context.

🇷🇴Romania amateescu

The bug is specific to Drupal CMS though :) Enabling the node entity type by default was recently changed in 🐛 Fatal when entity type doesn't exist (anymore) Active , and it works fine..

🇷🇴Romania amateescu

Re #6:

Should each trashable entity type have its own predefined View, or should we generate Views dynamically based on the selected entity type?

Generate Views dynamically based on the selected entity type.

When you say "on-demand," do you mean the Views should be created programmatically when accessed, or should they be predefined and stored permanently?

Views should be created programmatically when accessed.

Are there specific fields you'd like displayed for different entity types, or should the View dynamically adjust based on available fields?

The View dynamically adjust based on available fields, using the list of fields we currently display in the trash controller.

🇷🇴Romania amateescu

This is a great idea, thanks for opening the issue. Added a trash:restore command and modernized the Drush integration while I was there :)

🇷🇴Romania amateescu

amateescu made their first commit to this issue’s fork.

🇷🇴Romania amateescu

Needs work to handle redirect_delete_by_path() somehow..

🇷🇴Romania amateescu

I've been following and reviewing this MR for a while, and even though the workaround for pending revisions of menu links is a bit awkward, I can't see any other way around it.

Suggested a small improvement for the exception message, otherwise +1 for RTBC.

🇷🇴Romania amateescu

I started working on this feature as well from a different angle (Workspaces support) but didn't consider searching through the issue queue first.

However, after finding this issue, turns out my approach was almost identical, so I incorporated my changes into the existing work here :)

🇷🇴Romania amateescu

amateescu made their first commit to this issue’s fork.

🇷🇴Romania amateescu

Started a MR from a reroll of the patch in #25.

🇷🇴Romania amateescu

The test coverage seems quite heavy-handed for the issue we're fixing here. As mentioned in #5, I still think a simple method in a kernel test that calls WorkspaceAssociation::getTrackedEntitiesForListing() with $limit = FALSE would be better.

🇷🇴Romania amateescu

Wouldn't it make more sense to wrap those lines with $this->assertTrue() calls?

🇷🇴Romania amateescu

Re #12: Nope, it's now called "manually" from core/modules/workspaces/src/Hook/FormOperations::formAlter(). It's easier to see if you look at the commit directly: https://git.drupalcode.org/project/drupal/-/merge_requests/10447/diffs?c...

🇷🇴Romania amateescu

Re #11: Those test changes are reverting the wrong assumptions made in 📌 Revert and Delete actions should not be available for the current revision Active , which didn't take into account that it should be possible to revert to the default revision when there are pending revisions.

🇷🇴Romania amateescu

Nice, I'm glad you figured it out.

🇷🇴Romania amateescu

I just tried it locally by setting the auto-purge duration to 1 second, deleted a node which went to trash, ran drush cron and the node was purged from trash.

What do you mean by "run the trash cron job manually"? You can run Drupal's cron either from the UI (admin/config/system/cron) or using Drush (drush cron).

🇷🇴Romania amateescu

Is the problem happening on a fresh Drupal install or an existing site with other modules installed?

🇷🇴Romania amateescu

Is cron configured correctly for that site? Auto purging works by running a queue on cron.

🇷🇴Romania amateescu

@dziabodo, yeah I was a bit hasty with my "refactoring" for the purge and restore forms. For the moment I reverted those changes, so the current code from the MR should apply and work fine on 3.0.9. Now I need to rebase this on the latest -dev code.

🇷🇴Romania amateescu

Cleaned up the MR and merged into 3.x, thanks.

🇷🇴Romania amateescu

That's an interesting problem, I'll need to think a bit about what we can do to alleviate it.

Not sure I agree with displaying all trashed entities on a single page, in Add bulk restore function Active I'm trying to change those listings to use views where possible, and views can't work with multiple entity types :/

In the meantime, I mentioned this issue in #3500162-6: Define how to handle secondary local tasks in entity pages when Navigation Top Bar is enabled , to make folks aware of the possibility of having so many 2nd level tasks when designing the new Navigation Top Bar.

🇷🇴Romania amateescu

Committed to 3.x, thanks!

🇷🇴Romania amateescu

The fix looks good, and we should add test coverage for this somewhere in \Drupal\Tests\workspaces\Kernel\WorkspaceAssociationTest.

🇷🇴Romania amateescu

I think the test case laid out in the IS is an excellent starting point, thanks for working on this!

The error from #2 is caused by wse_workspace_presave(), which overrides the workspace ID with its UUID before saving. This is needed because core's assumption that workspace IDs should be reused after publishing doesn't match the expected content editor experience on most sites.

Until this is fixed in core, you should use \Drupal\Tests\wse\Functional\WseTestUtilities::wseCreateAndActivateWorkspaceThroughUi() instead of the core trait.

🇷🇴Romania amateescu

@adamzimmermann, I think it's fine to leave the refactoring of existing diff integration for a followup. If you didn't start it already, feel free to only do the other points from #7.

🇷🇴Romania amateescu

Merged into 2.0.x and cherry-picked to 1.0.x, thanks!

🇷🇴Romania amateescu

Adding a separate permission for managing preview links makes sense, but we should also allow the regular "manage any workspace" permissions. Updated the MR to account for them.

🇷🇴Romania amateescu

While this is still in the design phase, I'd like to point to an issue from the Trash module, which uses 2nd level tasks for each trash-enabled entity type. The problem raised there shows that there could be as many as 15-20 2nd level tasks, and it probably won't work very well with the proposal from #4.

🇷🇴Romania amateescu

I agree that having this feature would be super useful. I'd like to see it implemented in a very specific way if possible, by creating "on-demand" custom views for each trashable entity type. The reason for them being custom is that we need to display different fields than /admin/content for example.

🇷🇴Romania amateescu

The node log message works nicely because it's on the content logger channel, but if we're using the trash channel we need to specify the entity type as well.

NW for that and for failing tests.

🇷🇴Romania amateescu

Started a MR with a proposal for the new method. Feedback welcome :)

🇷🇴Romania amateescu

Opened the issue that was proposed in #129, but I quickly found out that we can't deprecate \Drupal\Core\Routing\RouteMatchInterface::getRouteName() because there are many legitimate use cases for it, as mentioned in that issue's summary.

Which leaves us with the PHPStan rule :) Since https://github.com/mglaman/phpstan-drupal is not managed by the core team, I think that rule can't be a hard blocker for this issue..

🇷🇴Romania amateescu

Are you sure you updated the module correctly? Because $base_path is only accessed on line 44 in RouteSubscriber: https://git.drupalcode.org/project/trash/-/blob/3.x/src/Routing/RouteSub...

Also, did you run any database updates?

🇷🇴Romania amateescu

No worries, we'll have to see if this problem comes up again in the future.

🇷🇴Romania amateescu

Merged into 3.x, thanks @denes.szabo for starting the work on this!

🇷🇴Romania amateescu

So, the fix is simple: if an entity has no link template for these actions, then do not provide them.

I'm afraid it's not that simple :) Trash support could be enabled only for certain bundles of an entity type, which would make the link templates available for it. What I missed in 🐛 Users without permission still see action links Active is that we need to forbid the restore and purge operations for entity types that are not trash-enabled.

🇷🇴Romania amateescu

That one sounds like it should be configurable.. I don't think either trashing or deleting the entities on the destination is the correct answer in all situations.

Can we add custom flags to the migration process/command? Like --trash=true|false?

🇷🇴Romania amateescu

I agree that rolling back a migration should delete the entities instead of trashing them. If Migrate has an API for hooking into the rollback process (before it starts), we need to set the ignore trash context like we do for similar use-cases, e.g. \Drupal\trash\EventSubscriber\TrashIgnoreSubscriber::onDefaultContentPreImport().

🇷🇴Romania amateescu

I liked a lot the idea of only storing the direct children of a term when it's trashed, but it still has the downside that we need an additional field to store them in, and I'd really like to avoid it if possible.

Thankfully, @eclipsegc had another idea: we could use the deleted timestamp that we already have, and restore child terms with the same timestamp as the parent. Since cascading deletes happen in the same request, all the deleted terms will have the same timestamp, so I think this approach is very feasible.

As for the trash_dependent_entities suggestion above, I'm not sure we need to generalize the idea of "dependent deletes" just yet, because it would only apply to taxonomy terms at the moment. Menu links for example, which also have a hierarchy, are not auto-deleting their children.

After looking at the MR posted by Ajit, I think there's something else we need to generalize first: we need to introduce "trash handlers", which would be responsible for all the entity type-specific customizations and hook implementations needed. Opened an issue for that: 📌 Add the concept of trash handlers Active , and postponing this one for now.

🇷🇴Romania amateescu

I saw that in the issue summary so I checked before doing it, and \Drupal\Core\Datetime\DateFormatter is very much translation-aware, most of its methods call formatPlural() at some point, so I think we're good.

🇷🇴Romania amateescu

amateescu made their first commit to this issue’s fork.

🇷🇴Romania amateescu

Then I need more info on the exact steps to reproduce, trash version, etc. because I tried the steps in the issue summary and I get a 403 when accessing node/X/delete?in_trash=1 or <code>node/X/delete?in_trash for a deleted node.

🇷🇴Romania amateescu

Implemented a fix for this, thanks for the bug report :)

🇷🇴Romania amateescu

amateescu made their first commit to this issue’s fork.

🇷🇴Romania amateescu

Committed a small fix because translation strings cannot be concatenated with variables, then looked into displaying the actual period of time until a trashed item can be restored, and it turned out quite easy to do :)

🇷🇴Romania amateescu

I think you have a module that clashes with Trash's node's access control handler override. Can you check that, for example using this code: get_class(\Drupal::entityTypeManager()->getAccessControlHandler('node'));?

🇷🇴Romania amateescu

Thanks for the bug report :) Can you try the MR attached?

🇷🇴Romania amateescu

amateescu made their first commit to this issue’s fork.

🇷🇴Romania amateescu

I also think it's fine to remove it, Navigation is still marked experimental.

🇷🇴Romania amateescu

@steffenr, note that the MR that was merged did not create a submodule, it fixed everything in the main module :)

🇷🇴Romania amateescu

I was also surprised to see them, even hardcoded before I changed that in Integrate with Workspaces Active , but didn't really look into why...

Posted a comment on the MR.

🇷🇴Romania amateescu

Merged MR !47 into 3.x, thanks for reporting and working on this!

Production build 0.71.5 2024