🇷🇴Romania @amateescu

Committed to 3.x, thanks for reporting it! I'll tag a new release as well :)

Oops.. this should fix it :)

We still need to handle deprecated routes somehow, per #5 and following discussion :)

Merged into 3.x.

Thanks for opening this (very detailed!) bug report :)

Automatically purging nodes would require a batch operation for that form, so I'd prefer to let the system work as intended and prevent the deletion of that content type instead.

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

Merged into 3.x.

Thanks for opening this issue. While I agree that this case needs to be handled better, I don't agree with the AI that generated the solution (and the issue summary by the looks of it), and I think we need to throw a helpful exception in order to make the developer aware of the problem.

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

Opened a MR with the fix and tests :)

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

Yup, that's the correct way to bypass trash and purge entities directly :)

Bulk purging (and restoring) will be implemented in ✨ Add bulk restore function Active :)

Oops, I wasn't looking at the changes tab. Not sure what happened there but I couldn't do anything with that branch so I opened a new MR instead.

@smustgrave, not sure why you think so, the MR was applying just fine. Merged the latest 11.x though just in case.

Merged into 3.x, thanks!

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

Merged into 3.x, thanks!

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

Merged into 3.x, thanks!

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

amateescu → created an issue.

No more test failures :)

amateescu → created an issue.

This was recently fixed here. You can use the dev version until a new release is tagged :)

I think this is still worth doing.

If it doesn't match, we can call RouteProvider::getRouteByName() on each of the passed in routes, this will trigger the deprecation we need.

@catch, the issue is that RouteMatch::isRouteName() will _not_ match the current route name in most (almost all) cases, so we'll be calling ::getRouteByName() all the time. Maybe it's easier to explain with an example:

Let's say we have a few different modules, and each one checks for a route, e.g. ::isRouteName('module1_controller'), ::isRouteName('module2_form'), etc.

Now, imagine that those checks happen on every request, wouldn't this mean that we'll be "attaching" the knowledge of those routes to every page in the context of the route preloader?

I don't think that subclassing the version history controller is a good solution for this issue.

The Diff module is a great example. From the perspective of another contrib module which also needs to alter the revision overview, and in order to support both core's controller and Diff's override, it has to subclass both of them. A third one with similar requirements would bring chaos :)

Adding a hook as in the current MR is one way to do it, but I think converting that controller to a form would be more helpful in the long run, especially since Diff (which is a very popular module) already does that.

@catch, yep, the space is added for a bit of padding, and it's handled correctly on RTL languages.

Merged latest 11.x.

Updated the test to use key-value instead of state.

@catch, copying FormController::getContentResult() wouldn't help us in any way, it's HtmlEntityFormController::getFormObject() that builds the form, and the MR is actually doing what you're suggesting, it copies the relevant parts from it into WorkspacesHtmlEntityFormController::getContentResult() :)

Yay, hook ordering actually works now :) Reverted that change and made EntityOperations::entityFormAlter() run first. Back to RTBC because there's no functional change to the MR and the current code has already been reviewed before.

amateescu → created an issue.

Merged into 2.x, thanks!

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

Opened a MR that implements #5.

Looks great now!

This is still relevant and not implemented yet :)

This is still needed.

Decided to set the initial workflow states with database queries. If any other viable entity storage gets written, we can worry about it then :)

Posted a few comments on the MR.

Try to clear any data you have in the workspace column of the paragraphs_item_revision table before running that update function. Something like this:

drush php:eval "\Drupal::database()->update('paragraphs_item_revision')->fields(['workspace' => NULL])->execute();"

You probably need to use the dev → version of the Diff module, because the latest tagged release at this time is 2.0.0-beta3, which doesn't contain the fix linked above.

You should run this update function using Drush or whatever other method you have available: wse_update_10001().

Updated the IS.

You need to upgrade the Diff module to a release that contains this issue: ✨ Allow usage of revision comparison inside workspaces Needs review

This is still a valid problem :)

This looks like a valid feature request still.

amateescu → created an issue.

@adamzimmermann

It feels weird to need a contrib module for core functionality to work though.

That's because core moves at a slower pace, and production sites needed a lot of various fixes for Workspaces that were lingering for years in the core queue for various reasons. Workspaces Extra had to incorporate all these little fixes to prevent developers from using a huge list of patches into their projects.

Opened a MR with a fix and test coverage for this issue.

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

Merged into 4.x, thanks!

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

I think this can be closed as a duplicate of 🐛 SqlContentEntityStorage::loadFromDedicatedTables()() runs a query that does not use indexes if an older revision is being loaded Active .

That code is now in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables(), and it has the same problem :)

amateescu → created an issue.

Addressed all the feedback so far, but there's one thing left.. the main reason for this API: checking whether the passed-in route name is deprecated.

The problem is that the only way to do that currently is \Drupal\Core\Routing\RouteProvider::getRouteAliases(), which would mean an uncached db query for every call to the new RouteMatch::isRouteName() method. I think we need a way to retrieve and cache (persistent and static) all aliased/deprecated routes somewhere in the route provider. Setting back to NW to get feedback on that.

The test failures are not caused by this MR, see 🐛 Fix failing PHPunit tests Active .

amateescu → created an issue. See original summary → .

Glad you were able to solve it.

Can you clear caches before running the database upgrade?

Did you run the database updates?

Whoops :) Merged into 2.0.x, thanks!

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

A workspace might have hundreds or even thousands of entities, so the current approach from the MR that loads everything won't work. We probably need to restrict the list to the first 10 (20?) or so entities of each type. And even then, the list will be incomplete so not really useful for its intended purpose.

Since you're (usually) coming to the publish form from the workspace overview page which shows you all these details, I'm not sure I see the usefulness of this feature tbh :)

I've committed this fix to entity_workflow, so we can now use hook_install() to insert this data.

I would set the draft state for open workspaces directly in the hook_install() implementation, and create a queue for populating the state of closed workspaces.

Thinking more about this, I think the issue should be moved to the entity_workflow module, and implemented like this in the entity_workflow_workspace submodule:

- implement a hook_form_alter that covers both entity_workflow_form and entity_workflow_simple_transition_form
- in that form alter, build and render the workspace publish form (directly for entity_workflow_simple_transition_form, using #states for entity_workflow_form)
- ensure that all form validators of the workspace publish form will run
- ensure that \Drupal\workspaces\Form\WorkspacePublishForm::submitForm() does not run (because the actual publishing will be done by the workflow transition)

This way, every customization/alter of the workspace publish form should be available in the workflow transition form.

Reviewed.

All the points from the issue summary are still valid, the documentation of \Drupal\Core\Field\FieldStorageDefinitionInterface::getMainPropertyName() doesn't tell developers what the consequences are if a field type has or lacks a "main property".

Looks great to me as well, thanks for sticking with it :)

Updated the MR on top of latest 3.x. Might still need some work around trash access -> entity access, so leaving the status at NW for now.

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

Merged into 3.x.

amateescu → created an issue.

Merged into 3.x. Thanks @plach for reviewing this MR!

amateescu → created an issue.

Merged into 3.x.

amateescu → created an issue.

Opened a followup for #19 and added a @todo pointing to it :)

amateescu → created an issue.

Thanks @krisahil, we're almost there! Added a comment to the MR, the data provider is not necessary and should be removed :)

Merged into 3.x.

Good idea :)