amateescu → created an issue.
I don't think we lose anything if we make them final. At the moment a hook implementation can be swapped via hook_module_implements_alter(), and when that will be removed its replacement will need to provide the same ability somehow.
So +1 for final :)
Thanks @plopesc for fixing the test! This is finally ready for review now :)
It's currently postponed on ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active per #97.
YAR! (yet another reroll)
Fixed :)
This should do it :)
amateescu → created an issue.
Why don’t I see removals of the new Hooks added to workspaces_ui to support toolbar, etc.
That was because of 📌 [ignore] Convert everything everywhere all at once Active , fixed in https://git.drupalcode.org/project/drupal/-/merge_requests/10147/diffs?c...
amateescu → changed the visibility of the branch 2972622-split-workspaces_ui to active.
amateescu → changed the visibility of the branch 2972622-split-workspaces_ui to hidden.
Started working on this.
And again :))
Rerolled again.
Cool, thanks for the confirmation :)
@thomas.feichter if I understood #3 right, it means we can close this issue, right?
Rerolled.
Thanks!
Fun fact: we had test coverage for this case, but the asserts where checking the Drupal frontpage assuming it would be the default content view from the Standard profile, but they were looking at the user page instead :)
Crediting folks from the private issue.
amateescu → created an issue.
Added a comment to the MR.
This was fixed in 🐛 [PP-1] Prevent content from being deleted when there is an active workspace Postponed because path aliases that have a default revision outside of a workspace can no longer be deleted.
Just closed 🐛 Slow query joining on content_moderation_state_field_revision Active as a duplicate.
Closing as a duplicate of 📌 Performance of moderation control state filter is slow Needs work .
Closing as a duplicate of 📌 Dynamically provide action plugins for every moderation state change Needs work .
Opened the followup.
amateescu → created an issue.
Yup, I only moved it to an OO hook hoping it would fix stuff, but it didn't.
Looks like just the hook_help generic test coverage is the problem now?
Yep, and I've already spent way too many hours last night trying to figure out what the problem is. The hook is there, I can access the help page in the UI on my local, but inside the test this line from \Drupal\help\Controller\HelpController::helpPage() returns FALSE:
if ($this->moduleHandler()->hasImplementations('help', $name)) {
Addressed the last feedback but this is sadly still blocked on a nightwatch fail that I've no idea how to fix.
While #2 is a desirable goal, I think the module needs to gain more real-world usage before we can consider moving it to be core API. For now, splitting the UI bits is an easier lift.
Committed and pushed to 2.0.x and 1.0.x, thanks! I'll also release alpha5 in a few minutes.
Merged into 1.0.x, thanks!
amateescu → made their first commit to this issue’s fork.
Merged into 2.0.x and cherry-picked to 1.0.x, thanks!
amateescu → changed the visibility of the branch wse-3485818 to active.
amateescu → changed the visibility of the branch wse-3485818 to hidden.
catch → credited amateescu → .
amateescu → changed the visibility of the branch 3420744-argument-1-id to hidden.
Committed the patch above to 2.0.x and 1.0.x.
Merged into 2.0.x and cherry-picked into 1.0.x, thanks!
amateescu → made their first commit to this issue’s fork.
This is now working nicely with the latest dev version of the Diff module, so I've committed the patch above to 2.0.x and cherry-picked to 1.0.x. Thanks everyone!
Needed to have a version of this MR that works on top of 🐛 "Unsaved changes" message incorrectly appears on layout builder Needs work , but I'm not sure how I can make that happen in the MR itself, so attaching a patch instead.
Definitely a good idea :) Added a link to the new README on the project page.
This a core problem, the workspace manage page doesn't list translations separately. But we can tackle it in WSE first and then move it up to a core issue.
Discussed with @plopesc and agreed that we should leave the "add the workspace navigation block by default" part to ✨ Allow modules to hook into top of content/footer sections of new core navigation Active . I'm not doing that myself right now because I see @finnsky is actively working on the MR :)
We also discussed test coverage, and since the current MR is just adding a button that does the same AJAX action as the on the in the Toolbar, there's nothing really testable at this point. Test coverage will be needed when we get to work on the full solution that will open the workspace switcher in the navigation popover.
This turned out to be a bug in a contrib module that wasn't actually publishing the workspace when it should have :)
After using the patch in #2 for a while, we realized that we actually need ✨ Convert Redirect entity to revisionable Needs work for proper workspace support, otherwise redirects can be created automatically when a site editor changes a path alias in a workspace, and that redirect is available in Live.. which is a bit problematic :)
@berdir, revision (and publishing status) support is also needed by Workspaces, because at the moment we only have the "ignore" approach from 📌 Allow CRUD operations for Redirect entities in a workspace Needs review , which basically means that any redirect created in a workspace is also available in Live, and that's not really a great (nor expected) behavior :)
For your concern about extra storage, I think we shouldn't create new revisions by default like the latest patch does, but allow this behavior to be configured for people that need it.
Committed to 2.0.x and cherry-picked to 1.0.x, thanks!
Merged into 1.0.x, thanks!
There's currently a single failing (Nightwatch) test left, and the problem seems to have been introduced in this commit https://git.drupalcode.org/project/drupal/-/merge_requests/9116/diffs?co.... No idea how to fix it, hoping that @finnsky can help :)
Sure thing, done :)
I think we should wait for the outcome of the discussion in 🐛 The method ContentEntityBase::getLoadedRevisionId() should not return string values Needs review since they're very much related.
What I would like for MongoDB is that revision IDs are returned as the correct type. When they are an integer they should be returned as an integer value, not a string value.
The problem is (and this applies to
📌
The method ContentEntityBase::getRevisionId() should not return string values
Needs review
as well) that it's not currently possible within the Entity API to type cast anything. And that's because $entity->get('revision_id')->value returns a string, and we can't really enforce a policy for not using that way of retrieving the revision ID. So even we change getLoadedRevisionId() and getRevisionId() to return integers, the MongoDB driver has no actual guarantee that's what it will receive in all cases.
I think a better way would be for MongoDB to typecast field values by itself when needed, but I don't know the codebase at all to make an actual informed proposal for where it should do that.
Yup, there's a @todo item about it: https://git.drupalcode.org/project/tmgmt_workspaces/-/blob/1.0.x/src/Wor...
The problem there was that we need to work with the return value of \Drupal\tmgmt\Form\JobForm::getConflictingItemsMessage() which is a PluralTranslatableMarkup() object and somehow append another string about workspace conflicts. I couldn't easily figure out the best solution and left it for later.
The tmgmt_job entity type is marked as workspace-ignored in tmgmt_workspaces_entity_type_build(), and this makes all its forms workspace-safe.
Is there a specific screen where you bumped into the workspace form limitation?
Using revision ids that are not an integer value is not supported by Drupal Core. The docblock of RevisionableInterface::getLoadedRevisionId() is clearly stating that the method only returns an integer value. What your client project is doing is NOT supported by Drupal core.
That's not actually true, the @return doxygen for \Drupal\Core\Entity\RevisionableInterface::getRevisionId() is int|null|string.
There is indeed a somewhat "implied" requirement that revision IDs are sequential integers (mostly with sort queries), but it's not strictly enforced. And UUIDv7 strings would also satisfy that requirement btw :)
There are a few problems here:
- why should we treat
getLoadedRevisionId()differently thangetRevisionId()? - the new argument defaults to FALSE but we update every usage of that method to pass TRUE. Wouldn't it be easier to not introduce the argument in the first place and only trigger the deprecation when
!ctype_digit($this->loadedRevisionId)?
amateescu → created an issue.
Merged into 1.0.x, thanks!
amateescu → made their first commit to this issue’s fork.
amateescu → made their first commit to this issue’s fork.
That's a very interesting problem to uncover!
I agree that uninstalling this module can be a "dangerous" operation, but sadly a hook_uninstall() implementation that would unpublish the deleted content is not feasible because there could hundreds or thousands of items in the trash, and the uninstall operation would just time out and leave the site in an inconsistent state.
I think a reasonable approach would be to *prevent* the module from being uninstalled if there is content in trash, and ask the user to either restore or purge that content before being able to uninstall it.
Oops, I meant to link 📌 Automated Drupal 11 compatibility fixes for wse Needs review . Thanks :)
Another big thank you from me as well! I couldn't have written all this in such a friendly yet concise manner :)
Committed the patch attached to 3.x!
amateescu → created an issue.
amateescu → made their first commit to this issue’s fork.
Fixed my review and merged into 2.0.x, thanks!
This should no longer be a problem with Drupal 10.3.
Duplicate of 📌 Automated Drupal 11 compatibility fixes for wse Needs review .