🇷🇴Romania @amateescu

quietone → credited amateescu → .

This could've been done in a new issue, but merged it here for expedience :) Thanks!

amateescu → changed the visibility of the branch 3045177-test-only to hidden.

Merged into 3.x.

There's no mention in #11 about testing with Layout Builder and Workspaces, so I tested everything myself.

Found and fixed one problem: when an entity using Layout Builder was purged, its inline blocks were only soft-deleted, basically leaving them around forever.

Did we consider doing this all in one expanding menu, and doing away with the confirm dialog?

Yup, that's the long-term plan per #30 :) While this issue is only focused on exposing Workspace's current switcher (with all its problems), in a followup we'll handle both suggestions. There's a screenshot in this comment #3457688-29: Support for core navigation experimental module → which shows the WIP design for the full Workspaces navigation UI.

Looks ready to me!

Replied to the MR comment.

Merged the second one as well :)

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

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

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

@andypost, thanks for fixing the pgsql issue, I was really scratching my head around it. Reverted your latest commit, we can't add them at the same time until the issue you just opened is fixed, and it's also not worth waiting for it :)

Updated the MR to use the new content_top region from ✨ Allow other modules to include their own Navigation blocks during installation Active . Another artificial dependency down...

The problem was fixed in https://git.drupalcode.org/project/drupalci_environments/-/commit/f07dd0...

amateescu → created an issue.

I have one idea that we can try, but it will complicate things a bit and it depends on content_moderation specifically.

If this idea works and it's not super hard to implement, I think it would be worth doing. It would be temporary anyway, until 📌 Allow for / implement simplified content workflow with workspaces Active is finished and Content Moderation will be able to properly track an entity and its dependencies.

we wouldn't even need to ask whether to re-parent the restored children once their parent gets restored, since they'd be restored alongside of it.

There's a tricky edge case here brought up by @Fabianx: what if a child term was trashed before its parent. In that case it shouldn't be automatically restored alongside the other children of that parent term. I've been thinking about this problem for quite a while, and I think it could be accomplished by ensuring that we only restore child terms with the deleted timestamp >= than the one of the parent term.

Merged into 2.0.x and cherry-picked to 1.0.x, thanks!

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

Note that the commits from #39 were not pushed.

Merged into 3.x.

amateescu → created an issue.

Service proxies are going away: 📌 Replace lazy service proxy generation with service closures Active

Opened an issue to remove _initialPublished from Workspaces: 📌 Fix usage of temporary entity data in Workspaces Active

Opened a MR to fix this. It doesn't need any new test coverage, this behavior is already tested in a lot of places.

amateescu → created an issue.

Merged into 2.0.x and cherry-picked to 1.0.x, thanks!

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

I think it's too late to backport this to 10.4.x now, and the regression pointed out above should get into 11.1.x since it's only a bugfix.

Rerolled.

I looked into it and the test failure is genuine, \Drupal\workspaces\Hook\EntityOperations::entityPresaveLast() runs before \Drupal\content_moderation\Hook\ContentModerationHooks::entityPresave().

What I found is that ModuleHandler::invokeAllWith() executes the callbacks in a foreach keyed by module name, and ModuleHandler::invokeMap['entity_presave'] looks like the screenshot attached.

Or perhaps it could be even simpler (at least in terms of UI, though not sure how tricky in terms of implementation) if there was a link to switch the site to a closed (previously published) workspace, same as how you can view a site in an open (not yet published) workspace?

We've been discussing this feature for a while, and in order to have a true representation of a past state of the site, there are two major problems to tackle in terms of implementation:

• deleted content: e.g. if a node is deleted after that workspace was published, we have no way of bringing it back unless the Trash module is also used, in which case this would be solved because the past revisions are still in the database.
• new content: e.g. if a new node is added after that workspace was published, we don't have a way of knowing that it should be excluded from being displayed, for example in views listings

Those problems also extend to entity types that are either ignored (e.g. files) or not supported by workspaces (not revisionable/publishable).

The latest "running thought" for this was to store the entire state of the site for workspace-supported entity types (i.e. a list of all entity IDs => revision IDs at the time of workspace publication), use that list to create a temporary workspace on-demand, and change the way of swapping default revisions to use an INNER join rather a LEFT join on the workspace association, which would ensure that only the entities that were "live" at that time will be queried/loaded/displayed/etc. This approach is a bit problematic in terms of database size though, which could grow _a lot_ for sites with a large amount of content, and it also doesn't handle ignored/unsupported entity types.

I think CPS in D7 does something similar, but it stores the list of entities in files on disk, which alleviates at least the database size issue.

The current code from HMIA doesn't run the implementation twice, it moves it first then moves CM's implementation before it. I'll look at the test failure tomorrow :)

Pushed a commit to fix Workspaces' presave implementation, we don't want to run the whole thing twice :)

Or , depending , one hook implementation which runs first and one that runs last -- we now have the ability to implement the same hook twice. Yay.

Yup, that's what we need for Workspaces, to split the current implementation in two separate methods.

workspaces is a fun one: It moves itself first *but* also moves content_moderation "firster". How would that look with the new system?

Workspaces needs the ability to implement the entity presave hook twice, one that runs first and one that runs last :) The current code that moves CM's implementation before is just a workaround.

I wouldn't mind changing wse_scheduler to use Scheduler's API/UI, or deprecating it entirely in favor of a Scheduler sub-module. The current code was a quick solution that "just worked", but I definitely agree that the UX is a bit.. lacking :)

Merged into 2.0.x and cherry-picked to 1.0.x, thanks!

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

Merged into 2.0.x, thanks!

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

Merged into 3.x :)

Opened 🐛 Clean up trash-enabled entity types when they're uninstalled Active to handle the uninstall part of this problem.

amateescu → created an issue.

Merged into 3.x, thanks for all the help here!

There's a @todo already in the latest commit from the MR :)

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

Merged into 3.x, thanks!

Fixed.

Rerolled.

What's the behavior of setting the inactive trash context when importing default content? Does the import process provide an error or something that it couldn't import a node because it already exists?

I defer the decision on how to handle this issue to the Drupal CMS / Recipes initiative folks, but my impression is that we should use the new API as it was designed: skip the content that can't be imported, and provide the reason for it.

Sadly the issue with modules providing fields for other entity types is not fully fixed, the Trash module tests also fail with a similar error like the one in the IS: https://git.drupalcode.org/project/trash/-/jobs/3799129

1) Drupal\Tests\trash\Functional\TrashNodeTest::testTrashAndRestoreNode
Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. Cannot add field 'node_field_data.deleted': table doesn't exist.

FWIW, adding container_rebuild_required: true to trash.info.yml fixes them.

I don't think we should take the route of providing a generic workspace-aware temp store implementation, we have #3026957: Provide a generic TempStoreIdentifierInterface that any object can use to better identify itself → for that, so let's keep this issue focused on the specific problem we have: LB's temp store needs to be workspace-aware.

Let's decorate the layout_builder.tempstore_repository service with a class that extends LayoutTempstoreRepository, and overrides its getKey() method to append the current workspace ID. Then test coverage needs to be added in \Drupal\Tests\workspaces\FunctionalJavascript\WorkspacesLayoutBuilderIntegrationTest.

I've read a bit about the change from https://www.drupal.org/node/3301716 → and the asset-surrounding code, and there's a @todo in \Drupal\Core\Asset\LibraryDiscoveryParser::buildByExtension() that will make core get the version information from the extension (module in this case), so let's not bother with maintaining that version string ourselves.

In other words, makes sense to me! Merged into 3.x, thanks!

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

Linking an issue from a long time ago that could've helped with this :)

Can't decide where to post this in the myriad of new issues opened, so I'll just do it here since it's related to #8.1.

Redirect's RouteNormalizerRequestSubscriber has a much bigger problem, it completely disables core's alias preloading mechanism: 🐛 Feature or Bug? - The path-alias preloading is not used if the "Enforce clean and canonical URLs" option is enabled Active

On the site where I discovered this (and had me digging through a lot of rabbit holes), we've disabled the Enforce clean and canonical URLs setting from the Redirect module, and went with this core change instead: #2641118-178: Route normalizer: Global Redirect in core → (MR link)

@catch, for #8.2, there's a very old core issue trying to change this behavior: 🐛 Aliased paths cannot be set as front page Needs review

That issue now competes with 🐛 Updating page.front Active for the decision whether storing an alias in config is ok or not :)

Added the remaining test coverage needed for \Drupal\Core\Routing\RouteProvider::getRouteAliases(), the MR is ready for final reviews now!

This is no different than entity types specifying \Drupal\views\EntityViewsData as their views_data handler, or even having entity type specific handlers (NodeViewsData extends EntityViewsData), without depending on Views :)

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

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

Merged into 2.0.x and cherry-picked to 1.0.x.

Merged into 2.0.x and cherry-picked to 1.0.x, thanks!

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

Looks good to me, thanks @phenaproxima! Left a comment on the MR, feel free to close it if you disagree :)

Trash could use that to say that an entity was skipped because one with the same UUID is in the trash bin.

amateescu → created an issue.

I think it would be better if we do this after 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work , with the benefit of having a lot less BC code and test coverage to maintain.

Added some test coverage in \Drupal\Tests\system\Functional\Routing\RouterTest but I think we need to add more in \Drupal\KernelTests\Core\Routing\RouteProviderTest, so leaving at NW.

@mikemadison, I've pushed a commit with another approach to the MR, can you try that one as well?

We've added a temporary safeguard for deleting translations in 🐛 Prevent deleting translations Needs work , and updated this MR to remove it :)

Merged into 3.x, thanks!

Posted an issue in the MR.

@mikemadison, is there any way to replicate your environment using DDEV? I'd like to get this problem fixed but it's kinda impossible without being able to debug it :)

These problems were fixed in 📌 t() calls should be avoided in classes Fixed .

Committed to 8.x-1.x, thanks!

Merged, thanks!

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

Reviewed the MR post-merge and the only thing I would've done differently is to not get the full list of workspaces that might track an entity (that can only happen when sub-workspaces are used atm), and pass TRUE as the second argument to the getEntityTrackingWorkspaceIds() call, which means that only one workspace will be returned. It's no big deal though, so I don't think it deserves a followup.

@cosmicdreams, the MR is handling all workspace-supported entity types. But it would be great if you could test it on your site for an additional confirmation.

Quoting myself from #3493461-16: Identify roadblocks to staging config in Workspaces (e.g., via wse_config) → :

That UI is basically just the result of code scaffolding when the wse_config entity type was created, and its current role is mostly for "debugging" purposes, it was never meant to be used by a content editor or such. The UI for viewing config that is changed in a workspace is the same as for content, the "Manage workspace" page.

I think it would be easier to just remove it :)

Workspace reverts were implemented in WSE with content in mind, config reverts haven't really been discussed nor tested that much, so they're pretty much a green field that can be changed in any direction needed by real-world use cases.

The design of workspace reverts in general is to allow users to revert workspace publications going back as far as possible, but only one at a time. For example, using the list of publications from #3, after the N workspace is published, then only that workspace can be reverted. If that happens, then the N-1 workspace publication can be reverted, and so on all the way up to the first publication.

In general, I get the feeling that wse_config is somehow seen as an immutable block of code, but it's really not.. it just happens to be the most experimental part of WSE, partly because it was a very hard problem to tackle, and partly because of lack of testing in multiple real-world scenarios.

About using config overrides for workspace-specific config, I've also been thinking about it for a long time, but, as confirmed by chatting with @alexpott a few days ago, that approach can't cover the use case of adding new config.

Is reverting broken for deep/complex reasons or is it a trivial(ish) bugfix?

I discussed this one with @alexpott in Slack and it seems to be a rather trivial bugfix.

I think I should have given more context to the conflict resolution remark - the moment you edit content in a workspace you can no longer edit it in the live site. The system prevents you from doing this. It's more conflict prevention than resolution I guess. The point is that the same it not true for configuration when you have wse_config installed.

Indeed, wse_config doesn't provide the same conflict prevention safeguards that content entities have, but it shouldn't be a big effort to add it.

If you visit the WSE Config listing page (admin/config/development/configuration/config-content) you will see a list entities changed in each workspace and these changes don't really relate to the change that's happened to the live workspace.

That UI is basically just the result of code scaffolding when the wse_config entity type was created, and its current role is mostly for "debugging" purposes, it was never meant to be used by a content editor or such. The UI for viewing config that is changed in a workspace is the same as for content, the "Manage workspace" page.

@catch summarized very well the current capabilities of wse_config in #3. Even though there might be some bugs here and there, any config that doesn't affect the database schema can be made editable in a workspace.

At the moment, the list of config that can be edited in a workspace is based on a "manual" allowlist, but a proper/long-term solution IMO would be to introduce something along the lines of FullyValidatable constraint from core that would work similarly to \Drupal\Core\Form\WorkspaceSafeFormInterface.

About field CRUD handling in a workspace, if that's not a strict initial requirement, I also think it would make sense to be discussed separately because it won't be nowhere as easy as a "adding a database table" discussion :) Hiding a new field from the UI would be the easiest part, IMO we should be more concerned about hiding it from the API. For example, we can't guess what kind of long-running (e.g. minutes or hours long) operations or other db schema alterations would be triggered by the "tiny" operation of adding a new field.

Apart from config changes that affect the db schema, which were "solved" by not being allowed in wse_config, the biggest challenge of that module (and any per-workspace-config solution in general), is the multitude of caching layers stacked on top of various config/services/plugin managers/etc. that have to be made workspace-aware.

That's in fact the ugliest and IMO hackiest part of the module, and I'd really love if we can come up with a better solution than what we have atm. One idea for it would be to introduce an alteration point at the very top of the caching system (in the cache backends or factories?), that would provide Workspaces an easy way to append the current workspace ID to the cache key.

That's an interesting use case and I agree it makes sense to provide a way to view all differences in a single screen :) Here's a few points on the approach:

- I'm not sure this needs to be on a separate screen, the table at the top duplicates the one from the main "manage workspace" screen, but it's a bit less user-friendly in my opinion (entity type and bundle machine names instead of labels, no pagination, etc.)
- I agree with this integration being in a sub-module, the main WSE module feels pretty crowded these days

With that said, how would you feel about these changes:

- move the inline diffs into the main "manage workspace" screen, and insert them in the table rows of each changed entity
- add a wse_diff.settings config to specify a "Show changes inline" boolean (FALSE by default), which would toggle between the current Diff integration from the main module (an entity operation) and this new "all in one place" view
- move the current Diff integration from the main module to wse_diff, and provide an upgrade function which installs the new sub-module if the Diff module is also enabled on the site

@mikemadison, can you check if the MR above fixes the problem for your environment?

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

While this issue was quite a nice trip down memory lane, I wasn't really serious with the previous service name :)

Updated it to "field purger" and brought this old patch up to current core standards, with a change record as well: https://www.drupal.org/node/3494023 →

That's a great failure, it shows that we're testing things properly. Fixed the update function.

Ohh.. that's right, somehow I didn't realize that only the main service is tagged :)

The kernel test failure seems to be caused by this change.