I think this can be closed as a duplicate of 🐛 SqlContentEntityStorage::loadFromDedicatedTables()() runs a query that does not use indexes if an older revision is being loaded Active .
That code is now in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables(), and it has the same problem :)
amateescu → created an issue.
Addressed all the feedback so far, but there's one thing left.. the main reason for this API: checking whether the passed-in route name is deprecated.
The problem is that the only way to do that currently is \Drupal\Core\Routing\RouteProvider::getRouteAliases(), which would mean an uncached db query for every call to the new RouteMatch::isRouteName() method. I think we need a way to retrieve and cache (persistent and static) all aliased/deprecated routes somewhere in the route provider. Setting back to NW to get feedback on that.
The test failures are not caused by this MR, see 🐛 Fix failing PHPunit tests Active .
amateescu → created an issue. See original summary → .
Glad you were able to solve it.
Can you clear caches before running the database upgrade?
Did you run the database updates?
Whoops :) Merged into 2.0.x, thanks!
amateescu → made their first commit to this issue’s fork.
A workspace might have hundreds or even thousands of entities, so the current approach from the MR that loads everything won't work. We probably need to restrict the list to the first 10 (20?) or so entities of each type. And even then, the list will be incomplete so not really useful for its intended purpose.
Since you're (usually) coming to the publish form from the workspace overview page which shows you all these details, I'm not sure I see the usefulness of this feature tbh :)
I've committed this fix to entity_workflow, so we can now use hook_install() to insert this data.
I would set the draft state for open workspaces directly in the hook_install() implementation, and create a queue for populating the state of closed workspaces.
Thinking more about this, I think the issue should be moved to the entity_workflow module, and implemented like this in the entity_workflow_workspace submodule:
- implement a hook_form_alter that covers both entity_workflow_form and entity_workflow_simple_transition_form
- in that form alter, build and render the workspace publish form (directly for entity_workflow_simple_transition_form, using #states for entity_workflow_form)
- ensure that all form validators of the workspace publish form will run
- ensure that \Drupal\workspaces\Form\WorkspacePublishForm::submitForm() does not run (because the actual publishing will be done by the workflow transition)
This way, every customization/alter of the workspace publish form should be available in the workflow transition form.
Reviewed.
All the points from the issue summary are still valid, the documentation of \Drupal\Core\Field\FieldStorageDefinitionInterface::getMainPropertyName() doesn't tell developers what the consequences are if a field type has or lacks a "main property".
Looks great to me as well, thanks for sticking with it :)
Updated the MR on top of latest 3.x. Might still need some work around trash access -> entity access, so leaving the status at NW for now.
amateescu → made their first commit to this issue’s fork.
Merged into 3.x.
amateescu → created an issue.
Merged into 3.x. Thanks @plach for reviewing this MR!
amateescu → created an issue.
Merged into 3.x.
amateescu → created an issue.
amateescu → created an issue.
Thanks @krisahil, we're almost there! Added a comment to the MR, the data provider is not necessary and should be removed :)
Merged into 3.x.
Good idea :)
Looks ready to me :)
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 :)
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.
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..
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.
This was fixed in Drupal CMS: 🐛 Trash link is a 404 on install because no entity types are enabled Active
Merged into 3.x.
This is a great idea, thanks for opening the issue. Added a trash:restore command and modernized the Drush integration while I was there :)
amateescu → made their first commit to this issue’s fork.
Duplicate of 🐛 Version 1.11.0 breaks on PHP 7.4 Active .
Needs work to handle redirect_delete_by_path() somehow..
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.
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 :)
amateescu → made their first commit to this issue’s fork.
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.
Wouldn't it make more sense to wrap those lines with $this->assertTrue() calls?
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...
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.
Nice, I'm glad you figured it out.
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).
Is the problem happening on a fresh Drupal install or an existing site with other modules installed?
Is cron configured correctly for that site? Auto purging works by running a queue on cron.
The fix in the MR is correct. This problem is caused by a recent Symfony commit: https://github.com/symfony/http-foundation/commit/32310ff3aa8126ede47168...
@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.
Cleaned up the MR and merged into 3.x, thanks.
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.
Committed to 3.x, thanks!
The fix looks good, and we should add test coverage for this somewhere in \Drupal\Tests\workspaces\Kernel\WorkspaceAssociationTest.
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.
@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.
Merged into 2.0.x and cherry-picked to 1.0.x, thanks!
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.
amateescu → made their first commit to this issue’s fork.
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.
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.
Looks great, thanks! Merged into 3.x.
amateescu → made their first commit to this issue’s fork.
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.
amateescu → changed the visibility of the branch 3506766-fix-entity-type-check to hidden.
amateescu → created an issue.
Started a MR with a proposal for the new method. Feedback welcome :)
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..
Opened a followup for this: 📌 Add an API for comparing the (current) route name that takes into account deprecated routes Active
amateescu → created an issue.
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?
No worries, we'll have to see if this problem comes up again in the future.
Merged into 3.x, thanks @denes.szabo for starting the work on this!
Last title change :)
I'm fixing this in 🐛 AI Assistant module error due to missing restore link template in Trash module Active .
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.