Problem/Motivation

The WorkspacePrePublishEvent subscriber in Workflows implements a query on tracked revisions that neither takes into account that there might not be tracked types, nor that there might be different entity types with the same revision id tracked.

On trying to publish a workspace which only has default state published content that is ready this causes:

  • in the first instance a fatal error because of a SQL syntax violation (see below);
  • and in the second instance an error message that incorrectly states that there is content in an unpublished workflow state.

Steps to reproduce

Automated tests

https://git.drupalcode.org/project/drupal/-/merge_requests/9062/diffs#di...

Manual tests

From OP on this issue:

  1. Enable the core content moderation, workflows and workspace modules
  2. Remove the default content moderation from all content types
  3. Add the default content moderation to the workspace entity type
  4. Create a new workspace or use the default Stage workspace
  5. Switch to the workspace and modify or create some content
  6. Try to deploy the changes from the workspace
  7. You should see the status message "Deployment failed. All errors have been logged."
  8. Content has not been deployed from the workspace.
  9. Logs will reveal no errors

From OP on #3132022: Content moderation check does not take into account entity IDs being used by two content entities:

  1. Enable the core content moderation, workflows and workspace modules
  2. Remove the default content moderation from all content types
  3. Add the default content moderation to the workspace entity type
  4. Create a new workspace or use the default Stage workspace
  5. Switch to the workspace and modify or create some content
  6. Try to deploy the changes from the workspace
  7. You should see the status message "Deployment failed. All errors have been logged."
  8. Content has not been deployed from the workspace.
  9. Logs will reveal no errors

Also more simply:

  1. Enable the core content moderation, workflows and workspace modules
  2. Switch to Stage
  3. Enable content moderation on two different entity types
  4. Create an entity of type 1 in an unpublished default state. To create an tracked revision id 1.
  5. Edit the entity to a published state. To create a tracked default revision id 2.
  6. Create an entity of type 2 in a published default state. To create a tracked published default revision id 1.
  7. Try to publish the state workspace.
  8. It will fail because no longer default revision of entity type 1 is in an unpublished state, but is reported matching for entity of type 2 revision 1

Proposed resolution

Add a check that there are tracked entity types, and include the entity type along with the revision id in the moderation state query. This is done in:

MR https://git.drupalcode.org/project/drupal/-/merge_requests/9062/diffs

Remaining tasks

Needs review.

Footnote

SQL error from OP:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')
GROUP BY base_table.revision_id, base_table.id) subquery' at line 2: SELECT COUNT(*) AS expression
FROM
(SELECT 1 AS expression
FROM
{content_moderation_state_revision} base_table
INNER JOIN {content_moderation_state_field_revision} content_moderation_state_field_revision ON content_moderation_state_field_revision.revision_id = base_table.revision_id
WHERE (content_moderation_state_field_revision.content_entity_revision_id IN (:db_condition_placeholder_0, ....)) AND ()
GROUP BY base_table.revision_id, base_table.id)
CommentFileSizeAuthor
#30 3179199-3132022-content-moderation-workspaces-query.patch7.49 KBekes
#20 workspace_content_moderation_check-3179199-20.patch6.72 KBadriancid
#18 3179199-nr-bot.txt1.65 KBneeds-review-queue-bot
#10 interdiff_9_10.txt4.51 KBacrazyanimal
#10 workspace_content_moderation_check-3179199-10.patch6.66 KBacrazyanimal
#9 interdiff_3_9.txt1.48 KBacrazyanimal
#9 workspace_content_moderation_check-3179199-9.patch1.96 KBacrazyanimal
#3 workspace_content_moderation_check-3179199-3.patch773 bytesacrazyanimal

Issue fork drupal-3179199

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

acrazyanimal created an issue. See original summary.

acrazyanimal’s picture

acrazyanimal commented
Issue summary: View changes
acrazyanimal’s picture

acrazyanimal commented
StatusFileSize
new workspace_content_moderation_check-3179199-3.patch773 bytes

Here is a patch to resolve this issue.

acrazyanimal’s picture

acrazyanimal commented
Status: Active » Needs review
acrazyanimal’s picture

acrazyanimal commented
Issue summary: View changes
acrazyanimal’s picture

acrazyanimal commented
Issue summary: View changes
sam152’s picture

sam152 commented
Issue tags: +Needs tests

Thanks for reporting and fixing this bug, it would be great to add a test for this problem.

acrazyanimal’s picture

acrazyanimal commented
Assigned: Unassigned » acrazyanimal
Status: Needs review » Needs work

I've actually dug a little deeper and tested a few other scenarios and it seems there are additional problems with this access hook implementation. So the patch is only a partial solution to the overall issue.

Within content_moderation_workspace_access() it first looks for all revision ids associated with a workspace. Then to determine access it checks what workflow states are associated with those revision ids. However, it does not check the states for the ids in combination with a condition on the entity type in question. It bulk checks all ids as a flat statement. The problem here is if there is a mix of entity types in the system using content moderation or not and some content managed entities being tracked in a workspace and some not, then the query can wrongly return results.

A couple real world scenarios come to mind:

  1. Both nodes and taxonomy terms are moderated but terms are being edited outside of the workspace and nodes within. You can get a false negative on the access check if any of the moderated node revision ids are the same as a term revision id that is in a non default revision state.
  2. A node which is tracked by a workspace but not using content moderation has the same revision id as a workspace that is being moderated. If the conflicting workspace is in a non default workflow state then the access check will fail even though it should be fine to deploy the node's changes.

Steps to reproduce

  1. Have an entity being tracked by a workspace be in a deploy ready workflow state, or a tracked entity that is not using content moderation. ie a node
  2. Have an entity of a different type (ie a term or workspace) that is not being tracked by a workspace but that is being content moderated be in a non deploy ready workflow state.
  3. If these two entities have the same revision ids and 'deploy content' is confirmed for the workspace, the deployment will fail with a message like "The Staged workspace can not be published because it contains 2 items in an unpublished moderation state."

Proposed resolution

This code will need to be refactored to check the revision ids in association with their respective entity type.

acrazyanimal’s picture

acrazyanimal commented
StatusFileSize
new workspace_content_moderation_check-3179199-9.patch1.96 KB
new interdiff_3_9.txt1.48 KB

I will see if I can create some tests for this issue. In the meantime here is a patch that contains a fix for the extended issue highlighted in #8 above. This patch replaces the existing condition on the bulk set of revision ids with grouped conditions that check revision ids AND entity type.

acrazyanimal’s picture

acrazyanimal commented
Assigned: acrazyanimal » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new workspace_content_moderation_check-3179199-10.patch6.66 KB
new interdiff_9_10.txt4.51 KB

Here is the patch from #9 but with tests. I added tests that cover the following scenarios:

  • A workspace contains only entities that are not moderated.
  • A workspace contains only entity types that are not moderated.
  • A workspace contains entities that are both moderated and not moderated.
  • A workspace deployment access check fails properly when the workspace contains an entity that is not moderated and an entity that is and is in a non-default state.
  • A workspace deployment access check passes properly when the workspace contains an entity that is not moderated and an entity that is and is in a default state. The workspace gets published.
  • A workspace contains an entity with a matching revision ID to an entity that is not tracked in the workspace, is moderated and not in a default state.
sam152’s picture

sam152 commented

Nice work @acrazyanimal, really thorough investigation and work.

Has the scope of this issue grown to include #3132022: Content moderation check does not take into account entity IDs being used by two content entities? We should either try to limit the scope of this issue and tackle the problems separately, or update the issue summary to describe all the scenarios that are being tested and the bugs that are being fixed.

In my experience, it's easier to get issues committed that expose a single bug and include a single focused test or test change to demonstrate it.

sam152’s picture

sam152 commented
Related issues: +#3132022: Content moderation check does not take into account entity IDs being used by two content entities
acrazyanimal’s picture

acrazyanimal commented

Thanks @Sam152. Yes I it does look like it covers the scenarios from the issue you linked above. Reviewing the comments in that issue's thread it looks like Alexj12 also increased the scope of that issue to cover the original issue here lol. I'm not sure what to do really. My inclination is to redefine this issue as an all encompassing issue since the tests are overlapping. I will think it over and possibly reduce this and contribute to the other issue.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3179199-nr-bot.txt1.65 KB

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented
Status: Needs work » Needs review
StatusFileSize
new workspace_content_moderation_check-3179199-20.patch6.72 KB

Reroll for 10.1

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

Will have to be rerolled for 11.x

Did not test or review yet.

ekes’s picture

ekes commented

@sam152
> Has the scope of this issue grown to include #3132022: Content moderation check does not take into account entity IDs being used by two content entities? We should either try to limit the scope of this issue and tackle the problems separately, or update the issue summary to describe all the scenarios that are being tested and the bugs that are being fixed.

@acrazyanimal
> Thanks @Sam152. Yes I it does look like it covers the scenarios from the issue you linked above. Reviewing the comments in that issue's thread it looks like Alexj12 also increased the scope of that issue to cover the original issue here lol. I'm not sure what to do really. My inclination is to redefine this issue as an all encompassing issue since the tests are overlapping. I will think it over and possibly reduce this and contribute to the other issue.

Looking I'm starting to get convinced https://www.drupal.org/project/drupal/issues/3132022#comment-15708738 that the query there is just going to throw too many false positives, for this, and the referenced issue, and for the other example I mention. So it might needs tweaking, not just filtering out revision ids.

ekes’s picture

ekes commented
Status: Needs work » Needs review

In creating the branch https://git.drupalcode.org/issue/drupal-3179199/-/compare/11.x...3179199... against 11.x I ended up with the same code as @acrazyanimal in #9 it truly does fix both #3179199 and #3132022 but rewrote the tests against the present state of the class and found that to make them clear they should be separated I think. The tests are in the first commit, so can easily be run to confirm they do fail with the present code.

ekes opened merge request !9062

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

ekes’s picture

ekes commented
Status: Needs work » Needs review

In the MR !9062 now. Which is passing tests.

ekes’s picture

ekes commented
Title: Content Moderation prevents workspace deployment when no tracked revisions are moderated » Content Moderation prevents workspace deployment
Issue summary: View changes
ekes’s picture

ekes commented
Issue summary: View changes
ekes’s picture

ekes commented
Issue summary: View changes
ekes’s picture

ekes commented
StatusFileSize
new 3179199-3132022-content-moderation-workspaces-query.patch7.49 KB

Adding a patch version of the MR for a composer patch ready snapshot that won't change.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to have 1 open thread for a small tweak.

ekes’s picture

ekes commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Believe the test needs to be updated too

1)
    Drupal\Tests\content_moderation\Kernel\WorkspacesContentModerationStateTest::testContentModerationWithoutDefaultRevisionsInWorkspaces
    Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException:
    You have requested a non-existent service "entity_type_manager".

Appears super close!

ekes’s picture

ekes commented
Status: Needs work » Needs review

Serivce name typo should also be fixed. Tests pass I think.

amateescu’s picture

amateescu commented
Status: Needs review » Needs work

Posted a few comments on the MR, once those are resolved I think this is good to go :)

amateescu credited Alexj12.

amateescu credited ravi.shankar.

amateescu credited renatog.

amateescu credited s_leu.

amateescu’s picture

amateescu commented
amateescu’s picture

amateescu commented

I've closed #3132022: Content moderation check does not take into account entity IDs being used by two content entities as a duplicate and transferred over the credit.

ekes’s picture

ekes commented
Status: Needs work » Needs review
yuvania’s picture

yuvania
she/her
Primary language Spanish
commented

I've tested the latest changes in my environment and everything seems to be working perfectly. Thanks everyone for the great work!

yuvania’s picture

yuvania
she/her
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One question on the MR.

ekes’s picture

ekes commented
Status: Needs work » Needs review

Hopefully clarified.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Tracked types doesn't seem bad. Debated "tracked content" but don't feel strong enough to request the change

ekes’s picture

ekes commented
Status: Reviewed & tested by the community » Needs work

Noticed some possible issues with workflows publishing. Going to put this on needs work until clarified.

ekes’s picture

ekes commented
Status: Needs work » Reviewed & tested by the community

It was an unrelated issue, not caused by this one. Behaviour here is correct.

  • catch committed 79054d8a on 10.3.x 
    Issue #3179199 by ekes, acrazyanimal, adriancid, smustgrave, sam152,...

  • catch committed 3704395c on 10.4.x 
    Issue #3179199 by ekes, acrazyanimal, adriancid, smustgrave, sam152,...

  • catch committed 998b1c6b on 11.0.x 
    Issue #3179199 by ekes, acrazyanimal, adriancid, smustgrave, sam152,...

  • catch committed 694cb802 on 11.x 
    Issue #3179199 by ekes, acrazyanimal, adriancid, smustgrave, sam152,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch closed merge request !9062

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.