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

Merge Requests

More

Recent comments

πŸ‡·πŸ‡΄Romania amateescu

Quoting @catch in #71:

My only (or at least main) objection is to storing things that are entity content as static props instead of field-backed, something which has already been implemented and is also being explicitly argued for in this issue and elsewhere.

This is also my main concern with the JSON-based storage proposed in this issue, and I'd really like us to arrive to a solution that stores most* data in individual entity fields. The goal of removing the indirection layer that Layout Builder and Paragraphs have with referenced entities is very much worthwhile, but going into the direction of storing everything in a blob is not much of an improvement IMO.. quite the contrary.

* By "most" I mean that the majority usage pattern for building 'layout pages' is to fill data for a predefined list of components (at least in my experience), while one-off components like the "Christmas hero" example mentioned above are the exception, and I hope we can find a way to store that data without having it "dictate" the entire storage model.

πŸ‡·πŸ‡΄Romania amateescu

Did you run the database updates after upgrading to 3.0.4? trash_post_update_set_enabled_entity_types_bundles() updates the trash.settings configuration to the (new) structure required by getEnabledEntityTypes().

πŸ‡·πŸ‡΄Romania amateescu

@Wim Leers, re #52:

Renumbering is the high-risk piece (i.e. the weak link) in this approach.

Note that \Drupal\Core\Field\FieldItemList::setValue() is not set in stone, field types can specify a custom ItemList class where that method can be overridden to support the use-case of assigning various field deltas to different components. Also, the other place that does renumbering is \Drupal\Core\TypedData\Plugin\DataType\ItemList::rekey(), and that can be overridden too :)

πŸ‡·πŸ‡΄Romania amateescu

FWIW, I added some `sleep()` calls in that code, executed drush cr and drush updb -y from the CLI, then browsed the site while the commands were still running, and I didn't bump into that error.

However, if you find that the MR makes a difference in your case, I've no problem with merging it :)

πŸ‡·πŸ‡΄Romania amateescu

@mparker17, did you somehow run trash_post_update_set_enabled_entity_types_bundles() twice by any chance? That would be a situation where the enabled_entity_types property of trash.settings would become an empty array, and \Drupal\trash\EventSubscriber\TrashConfigSubscriber::onSave() would then remove the deleted field from all previously enabled entity types.

πŸ‡·πŸ‡΄Romania amateescu

I've tried writing a custom filter similar to the one posted in #9 using "standard" entity reference fields, but I wasn't able to reproduce the situation where a field table was considered an entity base table.

This is the filter I tried:

/**
 * @ingroup views_filter_handlers
 *
 * @ViewsFilter("trash_test")
 */
class TrashTest extends FilterPluginBase {

  public function query() {
    // Join from daily availability entity to bookable entity (the actual room).
    $configuration = [
      'table' => 'node__field_tags',
      'field' => 'entity_id',
      'left_table' => 'node__field_image',
      'left_field' => 'entity_id',
    ];

    $join = Views::pluginManager('join')->createInstance('standard', $configuration);
    $rel = $this->query->addRelationship('node__field_tags', $join, 'node');

    // Join from bookable entity to loge entity.
    $configuration = [
      'table' => 'media',
      'field' => 'mid',
      'left_table' => 'node__field_media',
      'left_field' => 'field_media_target_id',
    ];

    $join = Views::pluginManager('join')->createInstance('standard', $configuration);
    $rel = $this->query->addRelationship('media', $join, 'media');

    $this->ensureMyTable();
  }

}

and defined like this in \Drupal\node\NodeViewsData:

    $data['node_field_data']['trash_test'] = [
      'title' => $this->t('Trash test'),
      'help' => $this->t('ABCD.'),
      'filter' => [
        'id' => 'trash_test',
        'label' => $this->t('XYZ'),
      ],
    ];
πŸ‡·πŸ‡΄Romania amateescu

FWIW, this MR missed adding an upgrade path for the new auto_purge configuration, I've just added it in https://git.drupalcode.org/project/trash/-/commit/42664da3d2d8e64702cb16...

πŸ‡·πŸ‡΄Romania amateescu

Rebased and merged into 8.x-1.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

This was eventually done in ✨ Implement core action plugins to support ECA, others Fixed , so I credited you over there. Sorry for taking so long to look at this issue..

πŸ‡·πŸ‡΄Romania amateescu

I've moved @akalam's commit into a separate issue: πŸ› Display only the operations the user has access to in subqueueListForEntity Fixed

@artemboiko, can you please write some steps to reproduce for this part?

we have a bug with configure user don't have access to this page but still can see link on entityqueue list page

Also, what's the use case for a separate enable/disable permission? Is the general "update" permission not sufficient?

πŸ‡·πŸ‡΄Romania amateescu

Good catch, here's a patch for this problem.

πŸ‡·πŸ‡΄Romania amateescu

The simplified workspace switcher has been rewritten and it seems to work well enough now.

πŸ‡·πŸ‡΄Romania amateescu

Cleaned up the code a bit and merged into 1.0.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

Oops sorry about that, I didn't look at the credit field when I merged it. Done :)

πŸ‡·πŸ‡΄Romania amateescu

@jwwj, is it possible to post the code of that custom filter in a way that doesn't expose any site-specific information? I think I understand the problem now, but I need to be able to replicate it locally in order to work on a fix :)

πŸ‡·πŸ‡΄Romania amateescu

This looks great! Fixed the tests and did a few small tweaks, and merged into 3.x, thanks!

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

All done!

πŸ‡·πŸ‡΄Romania amateescu

amateescu β†’ created an issue.

πŸ‡·πŸ‡΄Romania amateescu

No new feedback from the bot and D11 beta is out, so I think this issue can be closed.

πŸ‡·πŸ‡΄Romania amateescu

I've removed the custom implementation of SetInlineBlockDependency from wse_lb because it was assuming a specific workflow, where layouts are only edited in a workspace, and it wasn't really needed as well, setting the inline block dependency by revision ID works just fine in a workspace.

This patch is no longer needed, so I'm closing this issue.

πŸ‡·πŸ‡΄Romania amateescu

Linking the issue mentioned by @catch above.

πŸ‡·πŸ‡΄Romania amateescu

It seems this small change won't make it into this module, the core issue in Workspaces is getting fixed though, so closing this one.

πŸ‡·πŸ‡΄Romania amateescu

This is actually a bug because in HEAD, if you have Layout Builder enabled and edit an entity in a workspace, when you visit `/node/X/layout` in Live, the concurrent editing message is not displayed. Added a test for this problem and keeping the issue at RTBC because it's a simple test addition with no other functional change to the MR.

πŸ‡·πŸ‡΄Romania amateescu

Added a basic token validation for the query parameter workspace negotiator to comply with the interface description of the new method.

πŸ‡·πŸ‡΄Romania amateescu

Ok, let's do this, the change is harmless and allows the class to be instantiated :)

πŸ‡·πŸ‡΄Romania amateescu

In the service definition, the workspaces.association service must be marked as optional because it might not exist (for example when the Workspaces module is not enabled).

Note that this is not a bug because \Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber is an event subscriber class, and it shouldn't be instantiated unless it needs to react to the WorkspacePrePublishEvent event, which is only invoked when the Workspaces module is enabled. If some other module instantiates it.. well, it shouldn't :)

If we want to make a change in core though, we need to mark the argument as optional in \Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber::__construct(). Updated the issue summary to reflect that.

πŸ‡·πŸ‡΄Romania amateescu

Discussed a bit with @catch and I'll do a few changes to the MR.

πŸ‡·πŸ‡΄Romania amateescu

Well.. the MR doesn't change any functionality in Views, so I don't think it's needed here :)

πŸ‡·πŸ‡΄Romania amateescu

As for test coverage.. I don't think it's needed because this doesn't really change the behavior of workspace negotiators.

πŸ‡·πŸ‡΄Romania amateescu

Btw, a quick`n`dirty way to test this MR is to edit a couple of entities in a workspace and manually change WorkspaceViewBuilder.php::$limit to 1 :)

πŸ‡·πŸ‡΄Romania amateescu

Started a MR with a new approach that builds upon the entity form controller override that was added in πŸ› [PP-1] Prevent content from being deleted when there is an active workspace Postponed . That service is new in 10.3.x, so it doesn't need any deprecation handling :)

πŸ‡·πŸ‡΄Romania amateescu

Implemented the separate method as discussed above, and it's looks much better!

Regarding \Drupal\Core\Database\Query\TableSortExtender, we can't use it because the data displayed in the table is coming from the individual revision objects, not from the `workspace_association` table.

πŸ‡·πŸ‡΄Romania amateescu

That works if you have one image gallery with multiple images. What happens if you want two image galleries?

At the storage level, the first gallery can use deltas 0 to 3 (contains 4 items), and the second gallery can use deltas 4 to 6 (contains 3 items). Then at the `TypedData::get()` level we can assign each delta item to its specific component (gallery).

πŸ‡·πŸ‡΄Romania amateescu

It turns out that the work done in πŸ“Œ Add an API for allowing modules to mark their forms as workspace-safe Needs work was pretty much on point and useful for the use cases tackled by this issue.

Some changes in this MR might seem weird, so I'll add some comments on the MR itself to try and explain them directly in a code context.

πŸ‡·πŸ‡΄Romania amateescu

We can do something like this in WSE, but we also need an issue in the Diff module to factor out getting the revision IDs from \Drupal\diff\Form\RevisionOverviewForm::buildForm() into a separate method, because overriding and copying all that code is a bit too much to do in this module.

Once we have that issue (and patch), we need to extend this patch and override \Drupal\diff\Form\RevisionOverviewForm.

πŸ‡·πŸ‡΄Romania amateescu

@capita that's not possible currently because there's no integration between JSON:API and Workspaces. That's what we're trying to figure out in this issue :)

πŸ‡·πŸ‡΄Romania amateescu

Adding the url.path cache context to every page would be a performance regression, and we can avoid it if we use a lazy builder instead for the workspace toolbar tab, just like announcements_feed module does it.

I wasted a lot of time trying to add test coverage, with no success: https://git.drupalcode.org/project/drupal/-/merge_requests/7605/commits . Adding coverage for this should be as easy as adding a few more drupalGet() calls in WorkspaceToolbarIntegrationTest::testWorkspaceSwitch(), but our functional javascript test framework disagrees :/

My proposal is to go with only manual testing here, otherwise the issue will be stalled for who knows how many years...

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

πŸ“Œ Add an API for allowing modules to mark their forms as workspace-safe Needs work will make it possible for contrib/custom modules to declare their forms as workspace-safe.

πŸ‡·πŸ‡΄Romania amateescu

Opened a MR for it, and the bug can be easily tested by changing the existing coverage to do menu link operations via the UI, as it should :)

Btw, the problem is not only for sub workspaces, top-level ones have the same issue.

πŸ‡·πŸ‡΄Romania amateescu

I was 100% sure we had test coverage for this, so I looked into it and turns out it's a problem that can only be reproduced in the UI, and even though WorkspaceMenuLinkContentIntegrationTest is a functional test, it's doing CRUD operations on menu links via API calls :/

πŸ‡·πŸ‡΄Romania amateescu

Changed the approach to use two interfaces instead of a trait, and updated the issue summary and the change record.

πŸ‡·πŸ‡΄Romania amateescu

Moved the guide under "Core module" since it was marked stable in https://www.drupal.org/project/drupal/issues/3088643 πŸ“Œ Mark Workspaces as a stable core module Fixed

πŸ‡·πŸ‡΄Romania amateescu

I can't reproduce this with latest Drupal 10.2.x and wse, did you have any other wse submodules installed by any chance? I tried with wse_config, wse_lb, wse_menu all installed, and drush pmu wse still worked.

πŸ‡·πŸ‡΄Romania amateescu

@alexpott the idea with a trait came up specifically for the dynamic nature that some modules might need, like Layout Builder in core :)

The difference is that in HEAD, LB has to call a method in each form class for marking its forms as workspace-safe, while with the trait approach it only has to ensure that its method name matches the one from the "top-level" trait, and Workspaces takes care of calling it in its own form alter.

I don't mind going for the interface approach you suggested, Workspaces itself does that currently and it works great when there's no dynamism involved. Should we wait or ask for other opinions or do you feel (strongly or not) that the interface will be far simpler to implement for the majority use cases?

πŸ‡·πŸ‡΄Romania amateescu

Workspaces was just marked stable πŸ“Œ Mark Workspaces as a stable core module Fixed , so.. 8 years later it's finally time to close this issue! :D

πŸ‡·πŸ‡΄Romania amateescu

Even after 10.3, this module will still provide useful features that are not (yet) in core:

- move a piece of content from a workspace to another
- discard changes
- revert a workspace publication
- change menu hierarchies in a workspace with the `wse_menu` submodule

So I think the answer is "yes" :)

I'll probably need to open a 2.x branch that will only be compatible with 10.3+ because there are way too many changes to take into account for previous core versions.

πŸ‡·πŸ‡΄Romania amateescu

We also need to update that info for our test module, and it's probably time to drop support for 8.7.7 :)

πŸ‡·πŸ‡΄Romania amateescu

Since @catch was not a fan of the event approach, here's an alternative that would allow modules to declare their forms as workspace-safe even when the workspaces module is not installed.

I added the trait in the Drupal\Core\Form as an initial proposal, but it could also be added somewhere in the system module I guess..

Once Workspaces will be marked as stable, more and more modules will have to integrate with it, so I'm raising the priority of this issue.

πŸ‡·πŸ‡΄Romania amateescu

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

πŸ‡·πŸ‡΄Romania amateescu

amateescu β†’ changed the visibility of the branch 3208390-add-an-api to hidden.

πŸ‡·πŸ‡΄Romania amateescu

Posted a few comments on the 11.x MR. Otherwise this looks good to me :)

πŸ‡·πŸ‡΄Romania amateescu

Oops, I didn't realize we don't need that service anymore. Since the Merger class is not a service and it's also tagged @internal, I only removed the argument from its constructor, but I had to do the whole deprecation dance anyway for WorkspaceOperationFactory because that's an actual service :/

Also added a CR for it: https://www.drupal.org/node/3440755 β†’

πŸ‡·πŸ‡΄Romania amateescu

Turns out I was wrong in #2, the problem here is WorkspaceAssociation::onPostPublish() not deleting the associations of sub-workpaces.

πŸ‡·πŸ‡΄Romania amateescu

Hm.. that's right, I thought πŸ› Sub workspace does not clear Closed: duplicate was about the value of the workspace metadata field, but it's actually about WorkspaceAssociation::onPostPublish() not deleting the associations of sub-workspaces. I'll reopen it :)

πŸ‡·πŸ‡΄Romania amateescu

Just a quick note: we're removing the hidden dependency of workspaces on path_alias in πŸ“Œ Fix Workspaces' hidden dependency on the path_alias module Needs review .

πŸ‡·πŸ‡΄Romania amateescu

This will also help with πŸ“Œ Make "path_alias" module optional Needs work where the (current) MR took a shortcut and made the dependency explicit.

πŸ‡·πŸ‡΄Romania amateescu

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

Production build 0.69.0 2024