🇷🇴Romania @amateescu

Linking the issue mentioned by @catch above.

Looks good to me :)

Merged into 1.0.x, thanks!

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.

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.

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

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

@stefan.korn, I replied in #3411308-10: WorkspaceSubscriber service parameter $workspaceAssociation must be optional → .

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.

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

@longwave, good idea, done!

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

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

amateescu → created an issue.

Updated the issue summary.

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 :)

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 :)

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.

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).

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.

alexpott → credited 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.

@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 :)

Fixed both review points :)

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...

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

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

We can probably copy what Workspaces is doing: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/works...

📌 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.

It's definitely still an issue, I just closed 🐛 After closing workspaces dialog the vertical admin toolbar is displayed on top of workspaces interface Closed: duplicate as a duplicate of this one.

This is a duplicate of 🐛 The position of the toolbar moves above the off-canvas top dialog if a modal dialog is opened Needs work .

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.

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 :/

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

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

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.

@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?

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

Hiding old patch files.

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.

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

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.

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

amateescu → changed the visibility of the branch 3208390-add-an-api to hidden.

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

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 →

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

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 :)

Replied in the MR :)

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 .

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

amateescu → created an issue.

Opened a MR to fix those tests.

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

Added test coverage and revamped the issue summary since this can no longer be reproduced via the workspace conflict constraint.

FWIW, a similar problem was recently solved in the Field UI redesign ( 🌱 [Plan] Improve field creation experience Active ), and the final solution there was implemented in 📌 Consider replacing hook_field_type_category_info with YML based plugin discovery Fixed and 📌 Allow field_type_categories.yml entries to define asset libraries Fixed .

I also don't have access to that prototype, but, until it can be sorted out, there are a few screenshots in 📌 UX: Elevate the Workspace UI around the site canvas Needs work and a gif animation in #2732081-8: WI: Phase G2: Full-site preview with Workspace UI → , which demonstrate a large portion of the design concept for Workspaces UI.

There's also a recording of a UX meeting where that design was discussed: https://www.youtube.com/watch?v=hf8AovBZflo

Improved the patch a bit and converted to a MR, still needs tests so leaving at NW for now.

Marking as postponed on #2867707: WI: Phase H: Conflict management and local workspace merging support → .

This will be fixed in 🐛 Contraint validation error after merge: The content is being edited in the develop workspace Needs work .

This was recently fixed in 📌 Simplify the workspace conflict validator Needs work .

We also need to skip generating path aliases when a workspace is published, here's an update for that.

@catapipper, since 🐛 Regression: Do not bypass route access with 'link to any page' permissions for menu links Needs work was committed to Drupal 10.3, can you please test with that patch applied instead of the one from this issue and check if it fixes your problem?

WSE will need a big update to make it compatible with core 10.3, because all the of core patches that are included in the module have been committed. A lot of code will need to be removed in a commit that will make the module require 10.3.

Update the wording to match the current UI of the module.

Replied in the MR why I don't think an upgrade path is needed.

Fixed that last failure.

@andypost, yes, all the things mentioned in the issue summary are still left to do.

That SQLite test failure was great, it revealed that WorkspaceAssociation::getEntityTrackingWorkspaceIds() was relying on implicit sorting on mysql and postgres. Committed a fix for that.

@smethawee, have you also applied the patch for Entity Reference Revisions mentioned in #61?

Quick Edit has been moved to contrib: https://www.drupal.org/node/3259831 →

Pathauto generation works fine with the small patch from #3283769-8: Allow path generation inside of a workspace → , so I'm closing this one as a duplicate.

I think we should take a much simpler approach and just check if the Workspaces module is enabled, and allow alias generation if a workspace is active.

Yup, this was recently fixed in https://git.drupalcode.org/project/trash/-/commit/a54e043bb2eead2099a8df... :)

I'm pretty sure that if there was an actual problem you bumped into, it was probably 🐛 Views queries are not properly altered Fixed . Closing for now but please re-open if that's not the case.

Even though there are no steps to reproduce in core because the user's password field is not translatable, I think the patches and all the discussion in the issue highlight the deficiencies of PasswordItem, so I think the issue is still valid, just needs someone to pick it up :)

Duplicate of #3035089: Menu link hierarchy should be revision-aware → .

Note that the `wse_menu` submodule of the Workspaces Extra → module provides a temporary solution for this, by storing the entire menu tree per-workspace in a custom entity type.

Just wanted to point out that this issue really is outdated, all the code around adding fields from the UI was shuffled a lot in ✨ Make field selection less overwhelming by introducing groups Fixed and the followup issues.

📌 Simplify the workspace conflict validator Needs work ended up with the 'WI critical' tag because it adds the test coverage requested in #3091481: Add explicit test coverage for EntityWorkspaceConflictConstraintValidator → , so moving it up to the must-have list.

Also adding #2968165: Finish the Views integration → to the should-have list.

Now that the parent roadmap issue has only one "must-have" issue left, I think it's time to open the review process from core maintainers for this one :)

@alexpott because WorkspaceAssociation::getTrackedEntities() is a "low level" API function that shouldn't be influenced by any global pager. Maybe this is another point in favor of adding a separate method for this listing :)

@smustgrave, you don't need 50 workspaces, just to edit >50 entities in a workspace :P

One thing I don't really like about this MR is the large number of arguments of WorkspaceAssociation::getTrackedEntities(), but I didn't have any better idea. Maybe a new method is needed for getting a "paginated" list of tracked entities, without the ability to filter by entity type ID and entity ID... not sure.

Test coverage for this validator is being added in 📌 Simplify the workspace conflict validator Needs work . I've moved the 'WI critical' tag over there, so closing this one as a duplicate.

Converted the patch to a MR and added explicit test coverage, as requested by @plach in #3091481: Add explicit test coverage for EntityWorkspaceConflictConstraintValidator → .

All tests are happy now :)

Converted the latest patch to a MR, changed the approach for marking LB forms as workspace-safe by using a trait and marking individual forms, so this issue doesn't depend on 📌 Add an API for allowing modules to mark their forms as workspace-safe Needs work anymore.

Tests were added in #22, I just moved them to the Workspaces module, so removing the tag.

I think this is a good DX improvement, let's do it :)

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

Merged, thanks!