🇷🇴Romania @amateescu

Still interested...

This MR adds a lot of code for something that would be more easily done when we replace the trash overview pages with views. I'm sorry but I'm going to close this. The views overview work will probably happen in ✨ Add bulk restore function Active .

I know that the Group module does some pretty advanced query altering, but Trash's entity query alter is just adding a regular field condition, so I'm a bit surprised there's a possible clash.

Let me know whether you manage to reproduce this on a more vanilla site, otherwise I'll try to check it myself soon :)

Thanks for testing! Merged into 3.x.

While looking into @catch's suggestion and realizing that we can't use DeprecatedServicePropertyTrait because that's specific to variables that store services (objects), I figured out that we can actually provide a proper deprecation strategy for this. Wrote a CR as well.

Ready for reviews again :)

One solution could be for the system menu block to add a "fake" condition to the query (like system_menu_block = TRUE), and workspaces could check for that parameter condition before removing the enabled one.

catch → credited amateescu → .

Looks great, merged into 2.0.x, thanks!

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

Looks good to me :)

xjm → credited amateescu → .

Well.. that's very much internal to the content entity storage and I don't really see a reason for outside code/storage overrides to do something with it.

The alternative would be to add a new method specifically for clearing it, but I don't think that's preferable to the current approach from the MR. Core committers can decide I guess :)

xjm → credited amateescu → .

Added a CR.

xjm → credited amateescu → .

xjm → credited amateescu → .

xjm → credited amateescu → .

Okay, sorry about that :)

Right, but this request is for transferring the project ownership, that's why I only took @rötzi into account.

Still interested :)

I've tried to reproduce this quickly by putting this code in a template, and the output was correct, trashed nodes were not displayed:

{% if drupal_view_result('frontpage', 'page_1') is not empty %}
  <h2 class="section-heading">Publications</h2>
  {{ drupal_view('frontpage', 'page_1') }}
{% endif %}

Does your view have a filter on the deleted field? Because that's the only way to bypass Trash's Views query alter, see https://git.drupalcode.org/project/trash/-/blob/3.x/src/ViewsQueryAlter....

Opened another followup, this time a performance issue.

amateescu → created an issue.

You can create paragraph entities in a workspace if you have WSE installed or this patch, but they're not tracked by that workspace :) The CRUD part mostly means "allow any operation for this entity type but don't track it".

Merged into 3.x, thanks!

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

Agreed with #5, closing as a duplicate of ✨ Fixes to permissions Active

@mlncn, the problem you're bumping into is a core bug ( 🐛 ValidReferenceConstraintValidator should not try to enforce data integrity on pre-existing references Needs review ), and not related to this issue.

I've read through the latest comments and I agree that having access to enable/disable queues through the update permission is problematic.

However, I think it's an advanced enough use-case that we can use the administer entityqueue permission instead of adding a new one. Updated the MR to do that and also fixed a few other things.

Merged into 8.x-1.x, thanks for all the work on this!

I tried to reproduce this using the steps in the issue summary and I couldn't get the view to display anything after adding the second contextual filter. Can you please update the steps with actual values? Even node title and values for the body field would help to ensure that I'm testing your specific scenario.

And you can leave out the parts about adding the modules, installing the site, etc. as they're not necessary :)

amateescu → created an issue.

amateescu → created an issue.

Or XB would need to pick up the message from some generic source that Trash altered :)

That's exactly what needs to happen :) Trash alters the description message of \Drupal\Core\Entity\EntityDeleteForm (actually coming from its parent: \Drupal\Core\Entity\EntityConfirmFormBase::getDescription()), and XB's xb_page entity type declares that it's using that form, but the message from the screenshot seems to be provided by a JS file instead: ui/src/components/navigation/Navigation.tsx

Committed to 3.x, thanks for reporting it! I'll tag a new release as well :)

Oops.. this should fix it :)

We still need to handle deprecated routes somehow, per #5 and following discussion :)

Merged into 3.x.

Thanks for opening this (very detailed!) bug report :)

Automatically purging nodes would require a batch operation for that form, so I'd prefer to let the system work as intended and prevent the deletion of that content type instead.

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

Merged into 3.x.

Thanks for opening this issue. While I agree that this case needs to be handled better, I don't agree with the AI that generated the solution (and the issue summary by the looks of it), and I think we need to throw a helpful exception in order to make the developer aware of the problem.

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

Opened a MR with the fix and tests :)

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

Yup, that's the correct way to bypass trash and purge entities directly :)

Bulk purging (and restoring) will be implemented in ✨ Add bulk restore function Active :)

Oops, I wasn't looking at the changes tab. Not sure what happened there but I couldn't do anything with that branch so I opened a new MR instead.

@smustgrave, not sure why you think so, the MR was applying just fine. Merged the latest 11.x though just in case.

Merged into 3.x, thanks!

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

Merged into 3.x, thanks!

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

Merged into 3.x, thanks!

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

amateescu → created an issue.

No more test failures :)

amateescu → created an issue.

This was recently fixed here. You can use the dev version until a new release is tagged :)

I think this is still worth doing.

If it doesn't match, we can call RouteProvider::getRouteByName() on each of the passed in routes, this will trigger the deprecation we need.

@catch, the issue is that RouteMatch::isRouteName() will _not_ match the current route name in most (almost all) cases, so we'll be calling ::getRouteByName() all the time. Maybe it's easier to explain with an example:

Let's say we have a few different modules, and each one checks for a route, e.g. ::isRouteName('module1_controller'), ::isRouteName('module2_form'), etc.

Now, imagine that those checks happen on every request, wouldn't this mean that we'll be "attaching" the knowledge of those routes to every page in the context of the route preloader?

I don't think that subclassing the version history controller is a good solution for this issue.

The Diff module is a great example. From the perspective of another contrib module which also needs to alter the revision overview, and in order to support both core's controller and Diff's override, it has to subclass both of them. A third one with similar requirements would bring chaos :)

Adding a hook as in the current MR is one way to do it, but I think converting that controller to a form would be more helpful in the long run, especially since Diff (which is a very popular module) already does that.

@catch, yep, the space is added for a bit of padding, and it's handled correctly on RTL languages.

Merged latest 11.x.

Updated the test to use key-value instead of state.

@catch, copying FormController::getContentResult() wouldn't help us in any way, it's HtmlEntityFormController::getFormObject() that builds the form, and the MR is actually doing what you're suggesting, it copies the relevant parts from it into WorkspacesHtmlEntityFormController::getContentResult() :)

Yay, hook ordering actually works now :) Reverted that change and made EntityOperations::entityFormAlter() run first. Back to RTBC because there's no functional change to the MR and the current code has already been reviewed before.

amateescu → created an issue.

Merged into 2.x, thanks!

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

Opened a MR that implements #5.

Looks great now!

This is still relevant and not implemented yet :)

This is still needed.

Decided to set the initial workflow states with database queries. If any other viable entity storage gets written, we can worry about it then :)

Posted a few comments on the MR.

Try to clear any data you have in the workspace column of the paragraphs_item_revision table before running that update function. Something like this:

drush php:eval "\Drupal::database()->update('paragraphs_item_revision')->fields(['workspace' => NULL])->execute();"

You probably need to use the dev → version of the Diff module, because the latest tagged release at this time is 2.0.0-beta3, which doesn't contain the fix linked above.

You should run this update function using Drush or whatever other method you have available: wse_update_10001().

Updated the IS.

You need to upgrade the Diff module to a release that contains this issue: ✨ Allow usage of revision comparison inside workspaces Needs review

This is still a valid problem :)

This looks like a valid feature request still.

amateescu → created an issue.

@adamzimmermann

It feels weird to need a contrib module for core functionality to work though.

That's because core moves at a slower pace, and production sites needed a lot of various fixes for Workspaces that were lingering for years in the core queue for various reasons. Workspaces Extra had to incorporate all these little fixes to prevent developers from using a huge list of patches into their projects.

Opened a MR with a fix and test coverage for this issue.

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

Merged into 4.x, thanks!

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

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.