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

Merge Requests

More

Recent comments

🇷🇴Romania amateescu

Started a MR with an initial proposal. I don't love it but it's the best I could come up with :)

🇷🇴Romania amateescu

I think RTBC is the correct status when we have the backport MR ready.

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

@smustgrave, yup, and the test for it is in ::testMoveAllTrackedEntities() :)

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

Opened a new MR to fix the 11.2.x failing test.

🇷🇴Romania amateescu

Thanks! Published the new CRs.

🇷🇴Romania amateescu

Blocker is in, so I'll update this one asap.

🇷🇴Romania amateescu

Looks good to me now :)

🇷🇴Romania amateescu

I'm seeing the deprecations are changing in WorkspaceManager but believe these changes have already been in a release so is that okay?

I think it is. BC for constructors is done on a best-effort basis, and I was actually surprised I was able to do this change while preserving full BC :) The change here just means that classes extending WorkspaceManager that were updated for 11.2 will need to be updated again for 11.3 before 12.0.0 lands.

I see we are switching to cron and limiting to 50, which makes sense. My question is if those workspaces are large with lots of content each any concern using cron then?

The method we're deprecating was already ran by cron, we just moved the code around so there's no actual behavior change.

Do we need a CR for the new Event and Subscriber?

Right, I was waiting for the review thread for it to reach a conclusion. Added an additional CR: https://www.drupal.org/node/3553871

🇷🇴Romania amateescu

Left some feedback.

🇷🇴Romania amateescu

Here's the initial implementation issue: #3553775: Add a conflict management API

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

Postponed on 📌 Slim down WorkspaceManager Needs review .

🇷🇴Romania amateescu

Wondering, what is the planned upgrade path to populate the "provider" field?

It's already populated by workspaces_update_11303() with ->setInitialValue(DefaultWorkspaceProvider::getId()) :)

🇷🇴Romania amateescu

This is a very interesting proposal. We'll need to carefully consider the implications for it though..

🇷🇴Romania amateescu

@smustgrave, the issue summary says that we're not yet introducing the UI for this, that's why you couldn't find it :)

Anyway, this is blocked by 📌 Slim down WorkspaceManager Needs review now, so postponing.. hopefully for a short while.

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

Replied to the latest comments, and left the thread about checkAccess() open for core committers to give an opinion :)

🇷🇴Romania amateescu

Answered the new comments, thanks for being so thorough! :)

🇷🇴Romania amateescu

@geek-merlin, thanks a lot for the in-depth review! Addressed all points and wrote the change records.

🇷🇴Romania amateescu

Live-debugging this helped a lot :) Merged into 3.x, thanks!

🇷🇴Romania amateescu

After trying out the initial proposal here with a temporary field, I found that it's too limiting (as @geek-merlin also mentioned above), and that we need to go a few steps further and allow providers to easily control the behavior of entity operations in a workspace.

🇷🇴Romania amateescu

📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work got in, so this is no longer blocked.

🇷🇴Romania amateescu

We already have a view for displaying all entities in the trash.

We don't have a view, the listing pages at /admin/content/trash are generated "manually" in \Drupal\trash\Controller\TrashController::listing :)

🇷🇴Romania amateescu

@twod, can you please check if the change from 🐛 Update from 3.0.18 to 3.0.20 breaks D10 site with Draggable views Active fixed the problem for you as well?

🇷🇴Romania amateescu

QueryPluginBase::getEntityTableInfo() is a very nice find, exactly what we were missing :) Merged into 3.x, thanks!

I'll wait a bit before cutting a new release, maybe someone else can provide at least one more confirmation.

🇷🇴Romania amateescu

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

🇷🇴Romania amateescu

The MR is written on top of 📌 Add the ability to move content between workspaces Needs review , so postponing on that.

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

Decided to add the new method on the interface under the 1-1 rule, so we don't have to block other work on the followup.

🇷🇴Romania amateescu

Paragraphs shouldn't be tracked by workspaces. You should run drush php:eval "\Drupal::moduleHandler()->loadInclude('wse', 'install'); _wse_update_clean_entity_types_workspace_metadata(['paragraph']);" and then manually clean up any records in the workspace_association table that references paragraph entities.

🇷🇴Romania amateescu

Why are you opening a new MR?

🇷🇴Romania amateescu

The current MR works well for its purpose, but we're planning on adding a full admin UI for preview links. Sebastian is working on that so assigning it to him.

🇷🇴Romania amateescu

Note that this issue is closed, because it's not a problem in the WSE module. See #5.

🇷🇴Romania amateescu

Added a comment to explain why we also need to it here. This new ::setNewRevision() call is needed for the specific case of adding a new published entity in a workspace.

🇷🇴Romania amateescu

Fixed :)

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

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

🇷🇴Romania amateescu

Merged into 2.0.x, thanks!

🇷🇴Romania amateescu

What else am I supposed to do other than run entity updates?

Investigate why there are no database updates to run. In most cases this means you had a patch that provided an update function with the same number.

but every redirect on the site is disabled

Do you have a backup of the dev database _before_ running drush updb? If so, can you check the redirect table and see if there's an enabled field already?

🇷🇴Romania amateescu

@ronraney,

drush updb didn't do anything when I tried that

This probably means you previously had a patch applied which had an update function named redirect_update_8110(). You can follow the steps I detailed here to fix it: #3543872-2: Latest version of Redirect module failed to run updates.

Also, drush entup is never the solution to use for a production site, that's supposed to be used only in development when there's no entity data.

🇷🇴Romania amateescu

Closing this issue for now, but please reopen if you manage to reproduce it and provide more info.

🇷🇴Romania amateescu

I've been using Trash on a large site that uses Groups a lot and didn't bump into any problems, so unfortunately this one will need some more debugging on your end.

🇷🇴Romania amateescu

Merged the fix.

🇷🇴Romania amateescu

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

🇷🇴Romania amateescu

I think there was a problem with post_execute but I can't really remember :/ Maybe I just wanted to be sure that we restore the trash context as late as possible to account for any code that might deal with deleted entities..

I'll need to check if there's another way to do this at some point.

🇷🇴Romania amateescu

Merged into 3.x, thanks!

🇷🇴Romania amateescu

This is no longer needed after Convert Redirect entity to revisionable Needs work .

🇷🇴Romania amateescu

Yup, that's the fallback for any db that doesn't support it.

🇷🇴Romania amateescu

@smustgrave, I don't see how this can impact contrib, it's only adding functionality :)

🇷🇴Romania amateescu

While this patch can be used for sites that need it, I'd prefer waiting for Translation support Needs review to be finished instead of introducing more workarounds..

Also, uninstalling the module is not required, you can just disable trash for the specific entity bundle that you're dealing with. That way, the deleted field is not gone, and you can re-enable it afterwards.

🇷🇴Romania amateescu

Merged!

🇷🇴Romania amateescu

That's exactly the purpose of \Drupal\wse\Plugin\Block\SwitchToLiveBlock :)

🇷🇴Romania amateescu

I couldn't figure out why some of the original code was written as it was.

Looking at the issue where I wrote that code ( 🐛 Trash breaks revision-based views and relation queries Fixed ).. I can't figure out either :) But the change in this MR makes a lot of sense, we surely don't need to loop over all possible field table names of the deleted field if we're already looping through all the tables involved in the views query.

I also tested a revision view with the same setup described in the issue summary and it works fine.

Merged into 3.x, thanks!

🇷🇴Romania amateescu

Opened 🐛 Language-neutral path aliases are not deleted with the parent entity Active to fix a regression introduced by this issue.

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

This was fixed in #2233595: Deprecate the custom path alias storage where PathFieldItemList uses a single alias repository query in ::computeValue().

🇷🇴Romania amateescu

Decided to add full support path aliases instead: 📌 Add support for path aliases Needs review

🇷🇴Romania amateescu

amateescu created an issue.

🇷🇴Romania amateescu

Closing, WSE is up to date with Drupal 11.

🇷🇴Romania amateescu

The reason why WSE removes those operations is that it needs to enforce core's (current) workspace restriction policy: once you start editing an entity in a workspace, you can only interact with it in that workspace until it gets published to Live.

Since Parallel Workspaces removes core's restriction, I think it should also allow those two revision operations :)

🇷🇴Romania amateescu

amateescu changed the visibility of the branch 3548279-add-workspae-task-monitoring to hidden.

🇷🇴Romania amateescu

Do file and media entities still have to be excluded after the Twig fix?

And should we attempt to remove a similar optimization from \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent::$entityIdsToLoad here or in another issue?

🇷🇴Romania amateescu

Looking at function redirect_update_8110(), it does add the field and set the default value for the field -- but THIS ONLY AFFECTS NEW VALUES GOING FORWARD -- it does not set the value on existing redirects.

That's incorrect. redirect_update_8110() sets the initial value for the field, see line 258: https://git.drupalcode.org/project/redirect/-/blob/8.x-1.x/redirect.inst...

Are you sure the update function ran correctly? Or was the devel_entity_updates module used to "fix" the missing new field?

🇷🇴Romania amateescu

The core implementation of these methods will be vastly improved in Drupal 11.3 ( 📌 Refactor workspace association tracking to use a dedicated revision table Active ), so I think it's fine to revert them from WSE.

Merged into 2.0.x, thanks!

🇷🇴Romania amateescu

Merged, thanks!

🇷🇴Romania amateescu

Opened a sibling issue in the Trash queue: 📌 Integration with the Redirect module Fixed

Production build 0.71.5 2024