🇷🇴Romania @amateescu

Makes sense :) Merged into 2.0.x, thanks!

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

Merged to 2.0.x, thanks!

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

Blocker is in!

amateescu → created an issue.

Looks good to me.

Thanks for reviewing :)

Postponing on ✨ Add back 'disabled' redirects Active and ✨ Convert Redirect entity to revisionable Needs work .

Postponing on ✨ Add back 'disabled' redirects Active so the upgrade path doesn't clash. This MR uses redirect_update_8111() and the other issue uses redirect_update_8110().

Here's a patch from the current MR that applies on top of ✨ Add back 'disabled' redirects Active , for folks that need Workspaces support :)

Posted a few review points.

Decided to drop the behavior change for disabling redirects instead of deleting them on path alias updates, I think that should be discussed in a separate issue. This is fully ready for review now :)

The MR is ready now :)

Thanks @daffie for the review! Addressed most points in the MR, replying below to the other ones:

Can we use the upsert also for Drupal\Core\KeyValueStore\DatabaseStorage::setIfNotExists()?

Nope, because that method relies on the return status of the merge query, and upsert doesn't support that.

Can we replace all calls to Drupal\Core\Database\Query\Merge? If so, can we deprecate Merge?

Not sure, we can explore that in a separate issue.

The parameter $fields, is that only an array with values or can it also be an array with keys and values?

It's only an array with values.

Can we do some performance testing to make sure that with this patch Drupal becomes faster.

I ran time ddev drush si demo_umami -y three times before and after this change:

Before:
0,06s user 0,02s system 0% cpu 22,412 total
0,06s user 0,02s system 0% cpu 22,000 total
0,06s user 0,02s system 0% cpu 22,642 total

After:
0,05s user 0,03s system 0% cpu 21,223 total
0,05s user 0,03s system 0% cpu 20,941 total
0,06s user 0,02s system 0% cpu 20,550 total

So it definitely improves things :)

This was fixed in ✨ Fix Regressions - Block and Page Filter Active .

The MR looks good, thanks!

Made some code style fixes, added an upgrade path for the new config values and merge to 4.x

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

Thanks @avpaderno!

This should be fixed properly after some upcoming changes in Drupal 11.3, but in the meantime the quickest fix is to not purge any data when deleting sub-workspaces. Thanks for reporting this!

As far as I know the proposed resolution hasn't happened in another issue in the meantime, so this would be still be worthwhile :)

amateescu → created an issue.

Missed by one day, but still interested :)

Merged into 3.x, thanks everyone for the discussion and patience with this bug :)

Looked a bit into this and it seems like it's actually a core problem: CM sets the load_latest_revision = TRUE param even on the delete route, which I think is wrong.

Undoing that in a core issue might take a while though, so for expedience let's add a quick fix in Trash for now.

amateescu → changed the visibility of the branch 3320566-lawxen to hidden.

amateescu → changed the visibility of the branch 3.x to hidden.

An interaction with another module is my guess as well. I think what would need to happen for this error to appear is for an entity class to use the entity type manager instead of the entity field manager in FieldableEntityInterface::getFieldDefinitions(), in which case that entity class needs to be updated.

Closing for now but feel free to reopen if you bump into it again and can point to the problematic entity type.

I also can't reproduce this. What entity type were you trying to generate?

That's right.. I'll see what we can do about it.

This was recently fixed in 🐛 Deleting a workspace may lead to live data deletion Active , you'll need to update to the latest dev version :)

Merged into 2.0.x, thanks!

We decided to handle the root cause of this problem in WSE: 🐛 Deleting a workspace may lead to live data deletion Active

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.