As the module works now, if I create a moderated node in an unpublished state, that revision becomes the default revision until I either publish the node or transition to a state with the "Default revision" setting enabled. That has the somewhat confusing effect that the first time you create a node, the first draft will always be the default revision until you publish, which seems unintuitive. I suspect that the following scenario expresses the behavior most people would intuitively expect:

Given I create a moderatable entity in a non-published state (e.g., draft)
And I transition the entity to another non-published state (e.g., review)
Then the latest revision should become the default revision

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden created an issue. See original summary.

balsama’s picture

So are you saying that no revision should be the default revision until the initial entity is moved to a different moderation state? For example:

Given I create a moderatable entity in a non-published state (e.g., draft)

Then there is no default revision

And I edit the draft and save it as draft

Then there is no default revision

And I transition the entity to another non-published state (e.g., review)

Then the latest revision should become the default revision
Crell’s picture

The problem is that there MUST always be a default revision. if there isn't, node load breaks and with it the entire UI. :-) We ran into this issue while testing out the base functionality. Therefore, we explicitly set the first revision as default, because if we don't you can never load it again.

I agree it creates a bit of weirdness, and is especially problematic for blocks or other entities that don't have a "published" concept (as you cannot have a never-yet-public block). However, given the way core is architected I don't know of an alternative.

arknoll’s picture

@Crell

This is what is currently happening:

1. Entity is created and saved in "Draft" state <-- Default revision
2. Entity is updated and saved in "Review" state <-- NOT DEFAULT REVISION
3. Entity is updated and saved in "publish" state <-- Default revision
4. A new draft of the entity is created and place in "Draft" state <-- NOT DEFAULT REVISION

Can't we do this:

1. Entity is created and saved in "Draft" state <-- Default revision
2. Entity is updated and saved in "Review" state <-- Default revision
3. Entity is updated and saved in "publish" state <-- Default revision
4. A new draft of the entity is created and place in "Draft" state <-- NOT DEFAULT REVISION

Under both scenarios we will always have a default revision, its just the the default revision marches forward until it hits the publish state. After that point it sticks on the publish state until the latest revision moves back over to published. Then the default revision is updated.

Crell’s picture

In order to do that, we'd need to track if an entity had ever been published. While technically possible on nodes, Content Blocks simply don't have a published property. So we wouldn't be able to differentiate between steps 2 and 4, at least not in a scalable way.

saltednut’s picture

At the very least I would think one needs to accommodate or create a work around for #4 in the case where the original draft of an unpublished node should not remain the Default revision as further revisions get added on. I agree once published things get unruly as the default should remain tied to the published revision from that point further.

As it currently works, there is no possible way for Views to filter unpublished nodes that have a Latest revision set to 'needs_review' in the same way that Workbench did in 7.x.

If one did make the default moderation state for a new node as needs_review, then you'd see those in a "Needs Review" filtered View. However, we'd be seeing the original revisions, not the current (default) revision if say, the nodes were sent back to draft, edited and then again set to needs_review.

In addition, I have no Filter for "Is current revision" as we did in 7.x (happy to file another issue about this) -- so also limited there when trying to make a View of content revisions where I want to show only ones that are in a state of 'needs_review' in addition to being the Latest revision for their respective nodes. Again, citing the "Needs Review" View that was so important to Workbench in 7.x here...

Crell’s picture

Brant, can you file a new issue for the missing Views integration points? That does sound problematic, but separate from the issue being discussed here.

gabrielstudiogd’s picture

I feel like we should just stick to the way drupal 8 handle revisions / default revision, published/unpublished node :

1. Node is created and saved as unpublished AND revision is created <-- Default revision
2. Node is edited, set to published AND new revision is created <-- Default revision
3. Node is edited, set to unpublished AND new revision is created <-- Default revision
......

Everytime a new revision is created, it always becomes the default revision, accessible at the canonical entity route, no matter if node status is published / unpublished.

I understand some people might be interested in keeping a node published while creating a new draft (which is what happens now, since the default revision is always the latest published revision), but it makes more sense that this needs a workaround, and not the regular way node and revision works, so it could be something like :

1. Entity is created and saved in "Draft" state <-- Default revision
2. Entity is updated and saved in "Review" state <-- Default revision
3. Entity is updated and saved in "publish" state <-- Default revision
4. A new draft of the entity is created and place in "Draft" state <-- Default revision (so entity is unpublished)

arknoll’s picture

@Crell

"In order to do that, we'd need to track if an entity had ever been published."

I don't think we need to do this. I believe we can just do this:

Rule:
IF:
the current latest revision is not published
Or:
this Workbench Moderation transition is set to make default revision
Then:
make this revision the default revision

1. Entity is created and saved in "Draft" state
Run Rule: returns true = Default revision

2. Entity is updated and saved in "Review" state
Run Rule: returns true = Default revision

3. Entity is updated and saved in "publish" state
Run Rule: returns true = Default revision

4. A new draft of the entity is created and place in "Draft" state
Run Rule: returns false = Latest revision

"While technically possible on nodes, Content Blocks simply don't have a published property. So we wouldn't be able to differentiate between steps 2 and 4, at least not in a scalable way."

I think the 99% use-case for workflow is with nodes that do have a publish status. I don't think we want the perfect to be the enemy of the good here. In any event, I think the above rule would still work, the first condition would just never be met.

Crell’s picture

Assigned: Unassigned » josephdpurcell

gabriel: What you describe is what core does out of the box. The whole point of using this module is if you do *not* want core's behavior, that is, you want to have forward revisions. (Revisions that are not made the default when they are saved.) If you want the always-default behavior, uninstall this module and just use core. :-) (You can also mimic it by setting all of your states to "make default revision", although I still question why you'd use this module then.)

josephdpurcell and I just talked this issue over at length. We realized that what arknoll describes is actually possible, and... can work on non-nodes. The reason being that while blocks do not have a concept of published, the published state on nodes becomes a derived value off of the published flag on the state of the revision. That means we can look at the related state entity's published checkbox instead, and rely on THAT to implement the logic proposed.

Joe's going to take a crack at making that change, which should resolve the "first draft stays default until first publish" issue.

josephdpurcell’s picture

Here is a solution, but I hope it is not final without confirming the following:

We need the default revision on save to verify whether the default revision is published. To do this, currently I make a call to load the entity from storage. Is there no more performant way to do this?

If not, perhaps we should revise the method to only load the default revision if the first condition (!$entity->moderation_state->entity->isDefaultRevisionState()) passes, that way we do the expensive call only when we are certain we need to.

Status: Needs review » Needs work

The last submitted patch, 11: 2669830.patch, failed testing.

The last submitted patch, 11: 2669830.patch, failed testing.

The last submitted patch, 11: 2669830.patch, failed testing.

Crell’s picture

+++ b/src/EntityOperations.php
@@ -75,6 +75,18 @@ class EntityOperations {
+          $update_default_revision = true;

Drupal coding standards say TRUE, sorry. :-)

I suspect some of our integration tests will be affected by this change (and should be). Loading the entity only if we need to check it as suggested seems like a good approach. That said, perhaps there's something on ModerationInformation that could simplify the code a bit?

josephdpurcell’s picture

Tests should pass now.

Next steps:

  1. See if ModerationInformation can simplify the entityPresave method
  2. Write a kernel test for this new behavior

Status: Needs review » Needs work

The last submitted patch, 16: 2669830_16.patch, failed testing.

The last submitted patch, 16: 2669830_16.patch, failed testing.

The last submitted patch, 16: 2669830_16.patch, failed testing.

josephdpurcell’s picture

Hooray for functional tests!

It appears the issue is around language handling, but need to dig in.

[Edit from original comment: I spoke with an incorrect understanding, so I'm rewriting this comment.]

josephdpurcell’s picture

Alright, translations should now be handled properly.

There are still the two todos from comment #16, but want to make sure tests pass.

Status: Needs review » Needs work

The last submitted patch, 21: 2669830_21.patch, failed testing.

The last submitted patch, 21: 2669830_21.patch, failed testing.

The last submitted patch, 21: 2669830_21.patch, failed testing.

josephdpurcell’s picture

In attempting to resolve the test failures, I encountered a tangentially related issue, which I've filed as #2687789: Latest revision gives access denied.

I'm going to fix the tests for this ticket with the expectation that will be separate work.

josephdpurcell’s picture

There were a lot more changes to tests than I anticipated. Also, some of the tests I confess I didn't fully understand, and in other cases it appeared the test was incorrect to begin with--hence some of the tests are more modified than others.

Lets give this a whirl!

Crell’s picture

Status: Needs review » Needs work
+++ b/src/EntityOperations.php
@@ -70,6 +76,29 @@ class EntityOperations {
+              $default_revision = NULL;

If your code requires a NULL, your code is wrong. (95% of the time.)

The test changes look great, and I love how well-documented they are!

Unfortunately, this patch conflicts with #2672810: A newly created translation is lost, which I just committed. It changes the structure of the method in question, but in a way that should be easier to follow and maintain anyway. Can you reroll against 8.x-1.x? (Which should make the changes easier to review, and probably resolve the NULL, too. Score!)

josephdpurcell’s picture

I like how #2672810: A newly created translation is lost is written, so following the same pattern here.

Interdiffs are hard. So I diffed the #26 and #28 patches and am calling that the interdiff. I also included conflicts-28.txt which has the conflicts when trying to apply #26.

So, this isn't a straight re-roll, but the changes are hopefully simple to understand. I was able to eliminate NULL and the else statement that PHPMD doesn't like :)

josephdpurcell’s picture

Status: Needs work » Needs review
Crell’s picture

Just a few minor quibbles, easily fixed.

  1. +++ b/src/EntityOperations.php
    @@ -1,5 +1,10 @@
    +/**
    + * @file
    + * Contains \Drupal\workbench_moderation\EntityOperations.
    + */
    +
    

    Drupal coding standards changed recently to NOT have this redundant @file block here. We just removed them in #2685257: @file is not required for classes, interfaces and traits. No need to re-add.

  2. +++ b/src/EntityOperations.php
    @@ -113,17 +119,34 @@ class EntityOperations {
    +   * The default revision is the same as the entity retrieved by "default" from
    +   * the storage handler. If the entity is translated, use the default revision
    +   * of the same language as the given entity.
    

    Ahso, this explains why this issue incidentally fixed the other as well.

  3. +++ b/src/EntityOperations.php
    @@ -113,17 +119,34 @@ class EntityOperations {
    +    $default_revision = $storage->load((int) $entity->id());
    

    Strictly speaking, an entity COULD have a string ID. The (int) here is unnecessary in any case.

josephdpurcell’s picture

  1. Fixed. (Not sure how that got back in there!)
  2. Exactly :)
  3. Let's see if tests pass. If they do without the type cast, yay?
Crell’s picture

Status: Needs review » Fixed

Yay! Committed and merged. Thanks.

  • Crell committed b2fad03 on 8.x-1.x authored by josephdpurcell
    Issue #2669830 by josephdpurcell: Latest revision should become default...

Status: Fixed » Closed (fixed)

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

terminology’s picture

i wanna ask a question, are all the revisions of a node the same entity,
i added a Flag type"nodes" into a type of content,if i flagged one revision,i find i flagged all the revisions,
or do i need to add a flag " Flagging Flagging entity"?

i mean are all the revisions of a node the same entity or different entity?
can i just flag one of revisions?