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

Merge Requests

Recent comments

πŸ‡·πŸ‡΄Romania amateescu

Fixed the feedback from #41 and #43.

Local tasks are not broken for an aliased route, so I removed the change from LocalTaskManager and the new getUnAliasedRouteName() which didn't have a purpose anymore.

For aliasing user.well-known.change_password to user.edit, after reading https://symfony.com/blog/new-in-symfony-5-4-route-aliasing I think that's not a valid example, because the two routes are not pointing to the same (URL) path.

Wrote an upgrade path test, so we only need some test coverage for the new RouteProvider::getRouteAliases() method now.

As for triggering deprecation errors for local tasks, I don't think that's necessary, the current trigger_error() from RouteProvider::getRouteByName() is enough IMO. And combined with πŸ“Œ Add option to log deprecation errors to prepare for Drupal 11 Needs work , it should provide enough visibility for deprecated routes.

πŸ‡·πŸ‡΄Romania amateescu

There must be something else at play then, because I tried the snippet below and the following steps:

1. edit a node in a workspace, save
2. edit the node in Live, save
3. create a new workspace, and editing the node showed me the revision created in step 2.

--- a/core/modules/workspaces/src/Plugin/Validation/Constraint/EntityWorkspaceConflictConstraintValidator.php
+++ b/core/modules/workspaces/src/Plugin/Validation/Constraint/EntityWorkspaceConflictConstraintValidator.php
@@ -79,6 +79,7 @@ public static function create(ContainerInterface $container) {
    * {@inheritdoc}
    */
   public function validate($entity, Constraint $constraint) {
+    return;
     /** @var \Drupal\Core\Entity\EntityInterface $entity */
     if (isset($entity) && !$entity->isNew()) {
       $active_workspace = $this->workspaceManager->getActiveWorkspace();
πŸ‡·πŸ‡΄Romania amateescu

The expected behavior is that the most recent live revision is loaded and copied to the current workspace.

That's not the expected behavior in core at the moment. Editing the same entity in different workspaces requires a conflict resolution solution, which doesn't exist in core (yet?), that's why we actively prevent it.

If you'd like to give editors the ability to edit the same entity in different workspaces, with the (important!) caveat that whichever workspace is published last "wins" (i.e. the changes from the previously-published workspace will be lost), I'd suggest to override the plugin class of the EntityWorkspaceConflict constraint with a custom one.

πŸ‡·πŸ‡΄Romania amateescu

@SocialNicheGuru, are you installing Workspaces alongside other modules or just by itself? That route is declared in workspaces.routing.yml just like any others, there's nothing special about it, so I think this needs to be debugged locally to see which code is trying to load it before the router is rebuilt.

πŸ‡·πŸ‡΄Romania amateescu

@Darren Oh, I think you're the first one to actually bump into this problem that was only theoretical so far, so I'm wondering if you can share any more info about the code that is not workspace-aware and it was loading the wrong revision :)

As for the MR, let's switch the order in the condition and check for isEntityTypeSupported() before hasActiveWorkspace(), see πŸ› Fix workspace-support check in entity queries Fixed for a recent example why this matters.

πŸ‡·πŸ‡΄Romania amateescu

amateescu β†’ changed the visibility of the branch 3101671-11.x to hidden.

πŸ‡·πŸ‡΄Romania amateescu

amateescu β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡·πŸ‡΄Romania amateescu

@webdrips this patch only adds the possibility to "ignore" entity types in a workspace, it doesn't yet do it for the File entity type. That will probably happen in #3025785: Cannot create entity with image in non-default workspace β†’ .

Here's a fairly extensive update after using this patch in production for several years. The problem with the initial approach (using constants on WorkspaceInformationInterface) was that an entity type could not declare its workspace support when the Workspaces module wasn't installed, since the interface couldn't be loaded, so we're now using explicit entity handlers for supported and ignored entity types. Entity handlers can point to a non-existent class, because the class won't be invoked until someone tries to instantiate that handler.

Also added test coverage, so removing the tag. I'll convert the patch to a MR, then it should be fully ready for reviews!

πŸ‡·πŸ‡΄Romania amateescu

I've been thinking about this issue in the past couple of days, and I think there is a way to implement this cleanly thanks to the OO code that we didn't have in D7.

Here's my plan for how we could do this in D10+:

  • deprecate FieldStorageDefinitionInterface::isMultiple() and FieldStorageDefinitionInterface::getCardinality()
  • introduce FieldStorageDefinitionInterface::isStorageMultiple() and FieldStorageDefinitionInterface::getStorageCardinality() to replace them
  • introduce FieldDefinitionInterface::isMultiple() and FieldDefinitionInterface::getCardinality() that would default to the new storage methods
  • the new methods above also solve the problem of BaseFieldDefinition, which implements both FieldStorageDefinitionInterface and FieldDefinitionInterface, so the storage-level cardinality would still have to be specified, but each bundle-level field definition could use a different cardinality (equal to or lower than the storage-level one)
  • write a detailed change record to explain the difference between the storage-level and field-level cardinality, and when each of them should be used (hint: the field-level cardinality is the one that should be used in most cases)

This approach achieves the following goals:

  • before the existing methods are removed from core, contrib and custom module authors will get a heads-up that they have to change something
  • after the methods are removed from core (e.g. in 11.0), they will need to take this change into consideration and update their code accordingly

@Wim Leers, re #99, note that core already defines a Count constraint based on the field storage cardinality in \Drupal\Core\Field\FieldItemList::getConstraints(), and it would just need to be updated to use the field-level cardinality instead.

πŸ‡·πŸ‡΄Romania amateescu

In πŸ› Consistent entity delete access among entity forms, operations and actions Needs review we decided to drop the change to WorkspaceInformationInterface.

πŸ‡·πŸ‡΄Romania amateescu

Here's a patch for this.. needs proper testing.

πŸ‡·πŸ‡΄Romania amateescu

Given the patch that was committed here πŸ› Problems restoring content in a Workspace Fixed , and that the general approach of the module was recently switched to "never return deleted entities unless explicitly instructed", I think this is a genuine bug in the entity query alter at this point

πŸ‡·πŸ‡΄Romania amateescu

Committed to 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

Committed the patch above to 3.x.

πŸ‡·πŸ‡΄Romania amateescu

The approach from this MR won't work if you're using any other display plugin (for example Block), or if there's any other module that also changes the plugin class of the Page display.

I think it would be better to disable Trash during the view execution. Here's a patch for that.

πŸ‡·πŸ‡΄Romania amateescu

The MR looks good to me. Merged into 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

Committed the patch from #4 to 3.x, thanks!

πŸ› | Entityqueue | D10.1Beta
πŸ‡·πŸ‡΄Romania amateescu

Closing this issue because it couldn't be reproduced and no one else bumped into it.

πŸ‡·πŸ‡΄Romania amateescu

Merged into 8.x-1.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

This should do it.

This validation tested in \Drupal\Tests\content_moderation\Kernel\ContentModerationWorkflowConfigTest::testDeletingStateViaConfiguration(), but I don't think that adding specific test coverage for this bug would add any real value to that test.

πŸ‡·πŸ‡΄Romania amateescu

Funnily enough, I just implemented this functionality in the `wse_preview` submodule of Workspaces Extra β†’ . I don't think it's core material, so I'm going to move this issue over there.

For posterity, it was implemented in these commits: https://git.drupalcode.org/project/wse/-/compare/7f543447...d3f44f56?fro...

πŸ‡·πŸ‡΄Romania amateescu
  1. +++ b/trash.module
    @@ -203,8 +203,13 @@ function trash_form_alter(&$form, FormStateInterface $form_state, $form_id) {
    +  if (isset($form['description']['#markup']) && $form['description']['#markup'] instanceof TranslatableMarkup && ($has_trash_access || $has_restore_access)) {
    

    This means that the message won't change if the user can't access trash or can't restore entities. I think we still need to let them know that the entity is going to the trash rather than being (permanently) deleted.

  2. +++ b/trash.module
    @@ -212,12 +217,25 @@ function trash_form_alter(&$form, FormStateInterface $form_state, $form_id) {
    +        $form['description']['#markup'] = t($entity_delete_label, $params);
    ...
    +        $form['description']['#markup'] = t($entity_multiple_delete_label, $params);
    

    t() can't translate variables, we need to wrap each message above in t() calls :)

πŸ‡·πŸ‡΄Romania amateescu

I checked the code from the issue fork and it looks mostly ok, just this bit should be changed:

    $access = AccessResult::allowedIfHasPermissions($account, [
      "update {$subqueue->id()} entityqueue",
      'manipulate all entityqueues',
      'administer entityqueue',
    ], 'OR');

to

$access = $subqueue->access('update')'

Also, a MR should be opened from that issue fork so it can be merged :)

πŸ‡·πŸ‡΄Romania amateescu

Just merged the latest 3.x, and I'll try to review this again ASAP :)

πŸ‡·πŸ‡΄Romania amateescu

Isn't the real issue here that the menu link is still displayed even though it points to an inaccessible URL?

That's exactly the problem, and it's actually a core issue: πŸ› Regression: Do not bypass route access with 'link to any page' permissions for menu links Needs work

I deleted a node with a menu link, then checked the following:
- with the admin user, the menu link was still displayed
- with the anonymous user, the menu link was not displayed

Like @penyaskito mentioned in the MR, I doubt that deleting the associated menu link when a node is trashed is the right thing to do. There would be no way of restoring that node (if you're not using the `wse_menu` module).

There's also the option of unpublishing the menu link when the node is trashed, but that also has the problem that we can't know if we should publish it when the node is restored.

I'm inclined to close this as a duplicate of the core issue, any objections? :)

πŸ‡·πŸ‡΄Romania amateescu

I think it makes perfect sense to only show the last part of the message ("You can restore them from the trash at a later date if necessary.") if the user has permission to restore entities of that type :)

We should also change the first part of the message and not link to the Trash page if the user doesn't have the access trash permission.

πŸ‡·πŸ‡΄Romania amateescu

Committed to 3.x, thanks for finding and reporting this!

πŸ‡·πŸ‡΄Romania amateescu

This was caused by πŸ› Prevent trashed entities from being returned by entity_load Fixed , and as mentioned in the IS, not that hard to fix :)

πŸ‡·πŸ‡΄Romania amateescu

Merged into 1.0.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

Merged the followup as well :)

πŸ‡·πŸ‡΄Romania amateescu

Merged into 1.0.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

Converted the patch to a MR.

πŸ‡·πŸ‡΄Romania amateescu

@TwoD, thanks a lot for working on this! This functionality is very much needed, so it's definitely not going to be rejected :)

I agree with @penyaskito above, the MR looks mostly ok to me as well. The biggest concern I have is the need for creating default revisions when deleting an entity (or a translation), because that would completely break the Workspaces use-case which only creates default revisions when a workspace is published to Live.

Restoring a source language does not automatically restore all translations, but that is easily done from the entity's translation overview page.

We now have TrashStorageTrait::restoreFromTrash() (which wasn't available when this MR was created), can we restore all translations there? Or was there another reason for not doing it in the first place?

πŸ‡·πŸ‡΄Romania amateescu

@kala4ek, that's right, the new behavior is closer to the entity being actually deleted, which matches the scenario you described. Fixing the problem within \Drupal\Core\Field\EntityReferenceFieldItemList::referencedEntities() would have had the same outcome.

πŸ‡·πŸ‡΄Romania amateescu

The code is pretty straightforward, we can add tests if we bump into any bug :) Committed and pushed to 3.x.

πŸ‡·πŸ‡΄Romania amateescu

Looked a bit into this and it seems we need an event subscriber that reacts to the KernelEvents::REQUEST event, checks whether $event->getKernel() instanceof UpdateKernel and sets the trash context on the trash manager to inactive.

πŸ‡·πŸ‡΄Romania amateescu

Merged into 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

✨ Call hook during entity restore Needs review now handles all the hooks needed, let's keep this issue open to fix the problem with menu_link_content entities related to deleted nodes.

πŸ‡·πŸ‡΄Romania amateescu

I'd like to be a bit more thorough and match the core entity hooks for both soft-deletion as well as restoration :)

πŸ‡·πŸ‡΄Romania amateescu

Agreed with #7.

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

@penyaskito, great work here! I left a few comments in the MR, the biggest change I think should be the one about removing the limitation of auto-purging happening only in 'days'.

@EZKG, this feature is very close to being merged into the Trash module, so I think it's best to wait a day or two and then update to the latest version of the module :)

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

Merged into 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

Right, invoking a hook would be good, and we already have an issue open for it: ✨ Call hook during entity restore Needs review

πŸ‡·πŸ‡΄Romania amateescu

Oh I misread the problem you pointed out, it's about deleting nodes, not menu links.

Anyway, I still think that we shouldn't trigger the delete hooks, but handle it in a similar way with how we deal with menu_link_content deletions.

πŸ‡·πŸ‡΄Romania amateescu

Not triggering the delete hooks is by design, I tried this at some point and it created way more problems than it solved.

The issue with menu links should be handled by trash_menu_link_content_update(). Are you using the latest version of the module?

πŸ‡·πŸ‡΄Romania amateescu

I'm on the fence about this.. on one hand, a trashed node shouldn't be available to the system unless it is restored, so for that case it would be "works as designed" and the Devel tab shouldn't be visible at all(?), however the Devel tab might be useful for a developer or site builder for debugging purposes, so we might want to make an exception.

What do you think?

πŸ‡·πŸ‡΄Romania amateescu

Updated the IS and split up the remaining tasks into a single group so it's easier to scan what's left to do.

πŸ‡·πŸ‡΄Romania amateescu

Now that we don't return anything on entity_load, we need different approaches for trash routes (restore, purge) and for viewing trashed entities.

Committed the followup patch attached to 3.x.

πŸ‡·πŸ‡΄Romania amateescu

Committed the patch from #5 to 3.x.

πŸ‡·πŸ‡΄Romania amateescu

I've been thinking about this problem for a while, and finally decided to go in a (somewhat radical) different direction: Trash needs to prevent entity_load from returning trashed entities completely.

This was based on observations from custom code on various sites, and the problem that stood out the most was that there are cases where entity-derived data is displayed without going through access checks, for example directly outputting the result of count($entity->get('some_reference_field')->referencedEntities()), which internally does an entity_load_multiple, but doesn't run any access checks. This specific example could've been solved by overriding core's field item list class for ER fields, but then contrib/custom code that also override it wouldn't benefit from our changes.

This decision is also in line with the recent fix from πŸ› Trash breaks revision-based views and relation queries Fixed , which made all views (using either the data or the revision data as the base table) stop returning trashed entities by default.

πŸ‡·πŸ‡΄Romania amateescu

Tested on a few more views and I'm comfortable enough with those changes, so committed and pushed to 3.x, thanks everyone!

πŸ‡·πŸ‡΄Romania amateescu

Cleaned up the code a bit and committed to 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

The views integration is being overhauled in πŸ› Trash breaks revision-based views and relation queries Fixed , and the latest patch there fixes this problem as well. I'm going to close this one and transfer the commit credit.

πŸ‡·πŸ‡΄Romania amateescu

I just committed ✨ Add "Purge" operation link Fixed , so views of trashed entities should be fully functional now.

The reason for not providing a Views-based admin UI by default is because the list of "trashable" entity types is configurable, and views are not dynamic like that :)

πŸ‡·πŸ‡΄Romania amateescu

Committed to 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

I've been going back and forth through this code for the past week, and in addition to the problem described in the issue summary, it also doesn't handle entity references very well: a filter on the deleted field might be added to the view on either the base (data) table or the revision (data) table of the referenced entity type, and we're currently not checking for the second case.

Here's a patch based on the MR from #2 which fixes both problems and cleans up the class a little. I've done quite a bit of manual testing, but I'd love at least another confirmation if you have the time @TwoD.

I noted Trash does not modify entity queries if you tell them to look at all revisions, but I think it makes more sense to actually do this for Views.

I agree, and it makes sense to do the same for entity queries as well, but we'll need another issue for that.

πŸ‡·πŸ‡΄Romania amateescu

This should do it.

Adding a test for this one-line fix would be quite hard, because we'd have to simulate the conditions described in the issue summary and somehow add a safeguard for the infinite loop. Per 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC , I think we shouldn't require a test here.

πŸ‡·πŸ‡΄Romania amateescu

I did some thorough git archeology and the code block that this issue wants to remove from KernelTestBase::register() is there since the KernelTestBase class was added to core, #2336597: Convert path aliases to full featured entities β†’ just updated the comment inside that condition.

Also looked through the history of #3007661: Modernize the path alias system β†’ , and figured out that #3084983: Move all the code related to path aliases to a new (required) "path_alias" module β†’ and a couple of its followups are the issues that made the current MR possible. Additionally, the changes from this MR should help a lot with πŸ“Œ Make "path_alias" module optional Needs work , so +1 on the smaller scope :)

πŸ‡·πŸ‡΄Romania amateescu

Further testing of this patch revealed that it wasn't possible to select a layout template for an entity that was initially created without a template, so here's a fix for that.

@pradeepjha, maybe that's the problem you bumped into as well. In any case, for a entity that has a template selected, the Change layout template button can be found on the Layout tab, see the screenshot below:

Note that this patch still has to be applied on top of the MR from ✨ Support Layout Builder Restrictions RTBC .

πŸ‡·πŸ‡΄Romania amateescu

Entityqueue provides a Add item to a subqueue Rules Action, so if you're using the Rules module you probably can do a bulk insert of items. This could be copied into a core action plugin, which would be available as a bulk operation on the /admin/content listing. Patches welcome :)

πŸ‡·πŸ‡΄Romania amateescu

@Fernando Iglesias thanks for the notice! I released 7.x-1.6 β†’ for this :)

πŸ‡·πŸ‡΄Romania amateescu

Trash 3.x works by creating a new revision with the deleted field set to the timestamp of the deletion. Previous revisions are not changed in any way.

πŸ‡·πŸ‡΄Romania amateescu

It doesn't, but it would be a nice feature to have.

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

The 8.x-1.x and 8.x-2.x branches of this module are not supported anymore.

πŸ‡·πŸ‡΄Romania amateescu

The 8.x-1.x and 8.x-2.x branches of this module are not supported anymore.

πŸ‡·πŸ‡΄Romania amateescu

The 8.x-1.x and 8.x-2.x branches of this module are not supported anymore.

πŸ‡·πŸ‡΄Romania amateescu

The 8.x-1.x and 8.x-2.x branches of this module are not supported anymore.

πŸ‡·πŸ‡΄Romania amateescu

The 8.x-1.x and 8.x-2.x branches of this module are not supported anymore.

πŸ‡·πŸ‡΄Romania amateescu

The 8.x-1.x and 8.x-2.x branches of this module are not supported anymore.

Production build https://api.contrib.social 0.61.6-2-g546bc20