Allow 'syncing' content to be updated in content moderation without forcing the creation of a new revision

I have rerolled the patch to be applied for version 8.3.x

Looks like the tests don't agree we can remove this.

I've updated the patch to only enforce a new revision if the bundle is not configured to.

On to the point of detecting the change, could it be done purely at a UI level? We already have \Drupal\content_moderation\Entity\Handler\NodeModerationHandler::enforceRevisionsEntityFormAlter which ensures the checkbox is checked, maybe that's enough and we can assume any kind of programatic changes to entities will result in the new revision flag being respected?

'new_revision' and 'revision' would need to go in the entity handlers for the current approach, so NW anyway.

Why should be new revisions enforced? What if i would like to fix just small typo as an editor? I do not want to have 20 drafts created in 5 minutes by single editor.
Still searching proper way to get Content Moderation working like this image is working in Workbench Moderation in D7: https://www.drupal.org/files/images/workbench-moderation-screenshot.png
Patch in #5 simply works for me fine (even with those failing tests) together with small hook_form_node_form_alter() which removes #disabled from revision item in form.

And the same thing is about submiting moderation form - i really do not want to create new revision. Workflow != content.

@Sam152:

maybe that's enough and we can assume any kind of programatic changes to entities will result in the new revision flag being respected?

I don't think we can assume that because, without the setNewRevision() call that's being questioned by this issue, calling $entity->save() without also calling setNewRevision(TRUE) before will happily overwrite the existing revision without creating a new one.

@dawehner:

To be honest though I don't believe this is the right angle to solve it. We need some way to detect whether anything has explicitly changed this value. In case they did, we don't want to change the opinion.

That would imply making \Drupal\Core\Entity\ContentEntityBase::$newRevision a three-state property (NULL, FALSE, TRUE) with NULL as a default and making setNewRevision() fail if $newRevision is not NULL, which I think would be an API change.

I have another suggestion though:

\Drupal\Core\Entity\ContentEntityBase::setNewRevision() is kind of a one-way street in HEAD, it only handles the case when changing the value from FALSE to TRUE by removing the value of the revision_id field and preparing the translations for a new revision.

So how about making it also handle the case when we want to go from TRUE to FALSE, by setting back the revision_id to whatever value it had before (probably using getLoadedRevisionId()) and reverting the translation changes?

That way, the lingotek module for example could make sure that its preSave() code runs after Content Moderation so it will have the final decision whether a new revision should be created or not. This, however, has the disadvantage that the module which decides to not save a new revision also has to be sure (as in.. really sure) that there are no fields changed other than its 'metadata' fields.

We have #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously for doing something like that.

Now that #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously is in, we should re-ignite this issue.

Since #9 the solution will probably to a different direction. But I took a spin with #9 in 8.6.x and got the solution below. Perhaps it is useful to others.

  public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {
    /** @var \Drupal\Core\Entity\RevisionableEntityBundleInterface $entityBundle */
    // @todo Depenency Injection.
    $entityBundle = \Drupal::entityTypeManager()->getStorage($entity->getEntityType()->getBundleEntityType())->load($entity->bundle());
    // Enforce new revisions if configuration is setup correctly.
    if (is_null($entityBundle->shouldCreateNewRevision())) {
      $entity->setNewRevision(TRUE);
    }
    $entity->isDefaultRevision($default_revision);

    // Update publishing status if it can be updated and if it needs updating.
    if (($entity instanceof EntityPublishedInterface) && $entity->isPublished() !== $published_state) {
      $published_state ? $entity->setPublished() : $entity->setUnpublished();
    }
  }

I also encountered this issue. I am saving entities programatically, explicitly setting revisions on/off where necessary... however even when attempting to save without a new revision, it would always force a new revision.

#5 applied cleanly to Drupal 8.5.3 and fixed my issue.

As this breaks Drupal core API ( $entity->setNewRevision(FALSE) fails silently) I think this can be considered a major bug.

Patch #9 will break a programmatically saved entity with $entity->setNewRevision(FALSE);.
I agree with #21 that patch #5 is the way to go.

#5 will only work for node types where moderation is not enabled and if nothing else, we need tests.

I also think that new revisions shouldn't be forced on the API level, especially not when explicitly calling setNewRevision(FALSE) and not just for those that don't have that checkbox set aka don't use content_moderation on a given bundle/node type.

One more critical problem with such forcing: when we cancel a user with assigning the content to anonymous user, node_mass_update() functions loads all revisions and updates them. It's not expected that new revisions can be created by this process, but actually they are created for moderated content breaking the data in the database (e.g. revision_translation_affected field can become incorrect after that). So I suggest to treat the issue as critical.

Lets see what happens now if we try #5 again.

So those test fails are quite interesting, as we have all sorts of things no longer getting new revisions as the tests expect: forms, API tests, json_api tests, especially the form part seems interesting.

Seems like what we need first is a definition when exactly a new revision shouldn't happen and when not. The form test fails indicate that even with content moderation enabled, the UI doesn't always result in a new revision, seems like that definitely should happen.

One option would be to still default to always doing a new revision, but respecting an explicit FALSE, that e.g. node_mass_update() would need to set.

It's again really hard to make out the line between bugfix and API change. I feel like enforcing it like this *is* an API change (mass update nodes works fine => enable CM => mass update is broken), but it's also been like this forever when the module is enabled, so anyone who relies on it now could argue it is a BC break if we no longer enforce it.

One more critical problem with such forcing: when we cancel a user with assigning the content to anonymous user, node_mass_update() functions load all revisions and updates them. It's not expected that new revisions can be created by this process, but actually they are created for moderated content breaking the data in the database (e.g. revision_translation_affected field can become incorrect after that). So I suggest to treat the issue as critical.

While this sounds very broken, I'd expect node_mass_update() to create a new revision in this case. The main problem here is that in most circumstances we wouldn't allow a new default creation to be created via the UI if the node is in moderation.

For that particular case what is needed is the rough data change of all revisions, not the creation of a new revision, since we don't make changes which can be compared with some previous state in some past revision.

I think @Berdir is right re: the API concerns, forcing a new revision feels pretty fundamental to moderated entities at this point. Respecting an explicit FALSE might be viable, but @amateescu points out some problems with that in #15.

What about using \Drupal\Core\Entity\SynchronizableInterface instead, for something a little more explicit? Content moderation could dial back some of it's aggressive revision semantics if an entity is flagged as syncing. The name kind of sounds like it fits the lingotek and node_mass_update use case.

It does however sound like the real bug that should be addressed is:

breaking the data in the database (e.g. revision_translation_affected field can become incorrect after that)

Seeing what the impact of #33 is.

Respecting an explicit FALSE might be viable, but @amateescu points out some problems with that in #15.

Note that #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously was committed since then, so the better fix for lingotek at this point would be to ensure that it has a hook_entity_presave implementation that runs after CM so it can overrule the creation of a new revision.

However, the node_mass_update() case is a different problem because it doesn't handle only "metadata" field updates like lingotek, so I think it would be better to look into it separately.

As for the patch in #34, I think it makes very much sense to do that (as proposed in #3037136-2: Make Workspaces and Content Moderation work together as well), but not really as a fix for the problems mentioned above.

BTW, complex migrations are much better with the patch #27. No more useless multiple revisions, just one revision of every node! :)

I think it probably makes sense for #26 to be spun out into it's own issue.

I had another look and wrote some tests for this. I tried to capture and test what would happen while syncing and updating the moderation state: what happens to the state, publishing status and defaultness. The simplest approach that makes that most sense IMO is:

• A new revision will not be created.
• The moderation state can be changed during a sync.
• Changing the moderation state will result in the associated published status changing along with it.
• The default-ness of the revision will never be changed.

The last point is the pertinent one IMO. Setting an existing revision to be non-default, while not creating a new revision will not actually do anything at the storage level. Another revision must actually be promoted to be the default. Going through prior revisions, calculating if it was previously the default and reverting decision I think is complex, risky and may not necessarily be what is expected.

The downside of that approach is you could sync to a non-default status, for a default revision, but given this is an API level construct, I think it's an appropriate trade-off.

Adjusting the issue summary to the approach in #38.

Adjusting the comment and control flow to be a little more clear.

Just thought of another really good reason to exclude all changes to defaultness while syncing: syncing changes to previous revisions. Codifying that in a test.

Edit: the second uploaded interdiff is the correct one.

Wrong test base.

Just read through #38 a few times and I fully agree with it. Not creating a new revision or changing the defaultness while syncing is fairly obvious. I pondered a bit about allowing moderation state changes (accompanied by publishing state changes) and in the end I think that's very much needed, otherwise changing the moderation state during a sync process would bypass the moderation workflow of the entity.

The patch looks great as well!

So this mean in the custom code it's required to set syncing flag:

$entity->setSyncing(TRUE);

every time we need to save an entity:

$entity->save();

Otherwise the new revision will be created.

Didn't review the patch yet, just a quick comment that the the sync flag is also being discussed for #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) and there are several comments there that argue that using this generic flag isn't very intuitive.

This issue now only adds the API and a test for it, but I think we'll also need to review which cases in core need to use this flag now, like the mentioned revision author update process. That could be a follow-up, but then we'd need to create it in advance I think. And looking at the use cases might be useful before committing to a specific API/flag, as that might also help us decide if that makes sense in those contexts? Unlike the ChangedItem issue, the cases we have here need to work with and without content_moderation, so that's an important argument for using this generic flag.

A change record might also be useful?

@maximpodorov, I think it depends on a case-by-case basis. For example, migrations should probably use setSyncing(TRUE) by default, but, as @catch pointed out in #31, node_mass_update() probably shouldn't.

Thanks for the review @amateescu and @Berdir. Happy to add a change record.

Re: follow-ups, adding the flag to migration updates might be a good way to see this working in a real context.

Created the change record here: https://www.drupal.org/node/3052114

Logged one of the discussed follow-ups here: #3052115: Mark an entity as 'syncing' during a migration update.

I don't think the content moderation logic must dictate all over the core and contrib and custom modules what is entity syncing and what is not. We can come to the absurd by this way:
"every entity update is syncing if it's not the result of human action of entity editing (via entity form or mass actions on the content dashboard)".

Isn't it better just to return to simple solution #3 and use $entity->setNewRevision(TRUE) in the places where it's necessary for the content moderation logic?

I don't think the content moderation logic must dictate all over the core and contrib and custom modules what is entity syncing and what is not.

It doesn't, it just reacts to the fact that an entity save process declared itself as 'syncing'.

Isn't it better just to return to simple solution #3 and use $entity->setNewRevision(TRUE) in the places where it's necessary for the content moderation logic?

The 'simple solution' doesn't work. As soon as you enable content moderation for an entity bundle, the moderation workflow must be enforced at the (entity save) API level, otherwise we risk bypassing it when using regular entity API calls.

So (as I mentioned in #50) we have to review ALL the code which saves entities and add setSyncing() calls if necessary. Isn't it a very high price?

I suppose it's up to the caller to decide if a particular scenario meets the definition of an entity that is 'syncing'. So far with content and config entities, those are: deploying a workspace and importing configuration into a site.

Based on the original report that triggered this issue, I feel like pulling in translation data from an external service such as lingotek and adding it to individual revisions and translations also comfortably meets the definition of 'syncing', so the whole fix seems kind of appropriate to me.

I agree with Berdir on this one, this API is very generic.

Should we consider expanding setSyncing to work with bitwise flags instead of booleans?

$entity->setSyncing(EntityInterface::SYNC_OPERATION_MIGRATION);

/// And then.

$entity->isSyncing(EntityInterface::SYNC_OPERATION_MIGRATION);

Then we can move away from it being a binary flag?

I think there is more discussion to be had here, as seen in #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating)

Also, I'm not sure this is critical - which of the following does this cause?

• Render a site unusable and have no workaround.
• Cause loss/corruption of stored data. (Lost user input, e.g. a failed form submission, is not the same thing as data loss and in most cases is major).
• Expose security vulnerabilities.
• Cause tests to fail in HEAD on the automated testing platform for any supported environment (including random failures), since this blocks all other work.
• Cause race conditions, database deadlocks etc. for which even code with 100% automated test coverage may be affected

Happy to be wrong on that

I would rather make sure that SynchronizableInterface was documented thoroughly enough so it was:

1. Clear what semantics to implement as a developer handling a syncing or non-syncing entity in a save lifecycle.
2. Clear when it is appropriate for callers to flag an entity as syncing or not.

Then both module developers and callers have some more concrete guidelines in which to operate. I think adding extra flags to the method is essentially just creating N flags, instead of meaningfully defining what the current one we have is. The net result IMO would be more complex and less useful.

IMO, the current 'syncing' status for content entities is shaping up into a flag to indicate that a save is happening under conditions which are outside of a typical user initiated content update and that as much as possible, side effects from that save should be limited. Examples might be:

• Deploying a workspace.
• Batch updating old revisions.
• Running an update from a migration.

After defining some of those scenarios it becomes easier to rationalise eliminating some of the side effects that have been discussed (CM revision handling + changed timestamp).

Just my 2c, interested to hear what others think.

Very much agreed with all of #62 :)

Should we update the documentation of SynchronizableInterface in this issue or open a new one and postpone this on it?

Spun off #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities for consideration.

Another user was reporting #26 in another issue, so I have spun out #3062900: The combination of content moderation, translations and cancelling user accounts is broken..

Thanks @Sam152, for opening #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities! I think that can be discussed as a followup, since this patch is preventing progress in #3037136: Make Workspaces and Content Moderation work together, which is a stable blocker for Workspaces.

In think @Berdir agreed with going forward with the current approach in #51 (last sentence of the second paragraph), and we also have the requested CR.

Also, the suggestion from #60 (even if I don't necessarily agree with it, as mentioned in #63), could also be implemented in a followup because it shouldn't impact the overall solution for "CM should not be creating a new revision when the entity is in (some sort of) a syncing process".

Thanks for poking this again @amateescu. I think it makes sense to flip the dependencies and block the docs behind the other two issues, since they are used in examples in the latest patch: #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities.

It seems to me that the proposed solution does not fully address the issue originally reported by @dawehner: Don't force new revisions automatically in content_moderation. The current solution provides a way to skip the "offending" (no offense intended :) behavior only when an entity synchronization is happening, which makes sense but may not be the only case where avoiding the creation of new revisions might be needed, since synchronization is shaping up to specifically indicate an entity save not happening in the usual user-driven update context.

If we consider our famous example of an hypothetical autosave module relying on revisions, a possibility to save storage space could be to only create a new revision on the first autosave, while all the following ones could update the autosaved revision. This would be problematic if CM were enabled and I don't think it would be reasonable to mark the entity as syncing in this case, to avoid issues with CM, since this could have all sorts of unwanted side-effects.

All this to say that I'm concerned that the current patch and the accompanying CR seem to be promoting ::setSyncing() as the official way to skip new revisions in CM, regardless of context (see #50 for an example), which I don't think is correct. IMO the correct way would be to ensure that ::setNewRevision(FALSE) works, if called after CM. I agree with @amateescu when he says that we need to ensure that CM works correctly even in programmatic contexts (e.g. API first), so I'm not advocating changing the current behavior to respect a previous FALSE value, however a test proving that invoking ::setNewRevision(FALSE) after CM applied its logic indeed prevents a new revision from being created would be a good way to make the fix more complete. In this light the CR should be updated accordingly to promote ::setNewRevision(FALSE) as the "official" way and mention syncing as an alternative to be adopted in very specific conditions.

Thoughts?

As a side note, I agree with @catch in #31 that I would not expect node_mass_update() to update existing revisions, because for auditing purposes it would be better to keep (even limited) history when dealing with a user account cancellation. I think it makes sense to address that in its own issue especially if, as it seems, there are data integrity problems involved. This is why I suggested that #3062900: The combination of content moderation, translations and cancelling user accounts is broken. might actually be critical after all.

@amateescu and I discussed this in quite a bit of detail. I'll try summarise some of the points made during that discussion:

• The current solution does address the original issue, since both @amateescu and I agree that the lingotek use-case is covered by the semantics of using SynchronizableInterface, that is updating some historical revision with content pulled from some external system associated with a particular revision.
• Right now autosave and CM are incompatible until the whole revision tree problem space has been fleshed out, so I don't think it makes sense to use that as an example in this case. I'd imagine a working solution there will ship with it's own whole set of semantics and constraints that may or may not dramatically move the needle on how CM or autosave works entirely. I'm not completely up to date with this area, so happy to stand corrected.
• I think we should definitely clarify in the CR that CM is simply being updated to respect the syncing semantics, not that the need for squashed revisions should imply that something is syncing.
• There seems to be a strong correlation with users wanting to dial back new revisions and them actually doing some kind of sync operation. Here is an example of someone in the wild looking for a content moderation revision fix and trying to fix "changed" timestamps. By putting forward syncing as a solution here, I think it's increasing awareness of a relatively new API that most folks don't realise is at their disposal.
• Finally, with the updated test, I don't think we should really be promoting that this is supported by using a heavier weight pre_save hook for these reasons:
  • It's coupling intention with implementation details of content moderation.
  • It's not something you can control while simply saving an entity, so IMO it's immediately more complicated than what most implementors would want/expect.
  • It completely excludes content moderation from making any further data integrity fixes or adjustments based on the users intention in the future. By using a specific and known flag, if the system needs some alteration in the future, it'll be a far more limited scope to analyse than "a lot of folks have done custom CM-undoing things in latter entity lifecycle hooks.

Overall, I think there may be scope to explore this further outside of a syncing entity, but I think it would be best to explore in the context of a really concrete problem where we can: a) completely eliminate syncing as an option and b) interrogate the use case enough to rule out creating a new revision as the actual preferred option (as has been discussed with node_mass_update).

As a side note, CM being somewhat opinionated and overriding certain standard behaviors was part of the design, see from 3 years ago: #2839371: Programatically creating a published entity doesn't result in a published entity.. I know @alexpott and I also discussed and came to terms with this in other contexts as well.

BTW, is migration a syncing process?

Thanks for the summary @Sam152 :)

tldr; below more details follow:

• I'm fine with not promoting the ::setNewRevision(FALSE) solution as the "official" one in the CR (yet). However, I think we need a closing paragraph honestly stating the current status: i.e. syncing is a new API and you need to carefully evaluate whether using it, especially because it might have unexpected side effects, as core/contrib adopt it over time. The CR should also mention the alternative solution "in case syncing fails or does not apply" together with the drawbacks outlined in #69.
• I still think we need some kind of test to verify that acting after CM works. I added both "needs" tags, but actually it's an either/or, although I'd prefer automatic tests.

Most points expressed in #69 make sense to me but overall they do not fully address the concerns I expressed in #68, since they are basically saying "we don't know whether there is a valid use case for skipping new revisions besides syncing". Meanwhile hypothetical devs with this use case might abuse syncing to get their logic working. If I were to write custom code (or contrib explicitly supporting CM), I would be fine with hardcoding a few assumptions about the system internals to make the logic work, even if that requires messing with hook weights. This is not an ideal solution but it's supported and it's been relied upon since D7 (or even earlier in some more primitive form).

Right now autosave and CM are incompatible until the whole revision tree problem space has been fleshed out [...]

That's why I talked about the "hypothetical" AS module: it's just an idea of how a revision-based AS module could work. The same module we used while working on the revision tree design. In that context, the AS module does work (at least theoretically) with CM and WS, and I think the use case I described is valid because the revision tree implementation, as we envision it so far, would not address that specific problem.

I think we should definitely clarify in the CR that CM is simply being updated to respect the syncing semantics, not that the need for squashed revisions should imply that something is syncing.

+1

There seems to be a strong correlation with users wanting to dial back new revisions and them actually doing some kind of sync operation.

True but we might also have a counter-example: node_mass_update() does seem to fit our current definition of syncing operation, however we would still need new revisions to be created (not sure about updating the changed timestamp). Luckily we would have to explicitly set the new revision flag to TRUE anyway, since we cannot rely on CM being enabled.

Overall, I think there may be scope to explore this further outside of a syncing entity, but I think it would be best to explore in the context of a really concrete problem [...]

Makes sense to me, but I think we need some kind of "temporary" solution until we flesh this out.

I'm happy to write extra content for the CR that clarifies the situation and developmental status of the whole syncing paradigm.

If I were to write custom code (or contrib explicitly supporting CM), I would be fine with hardcoding a few assumptions about the system internals to make the logic work

I still feel weird about writing and supporting a test that codifies implementation details of CM, I feel like it sends the wrong signals to implementors. For modules that reach into other modules implementations or swap low level components out, IMO they really need to be testing, understanding and supporting the impact of those low level changes as well as being prepared to chase HEAD on those changes if they are ever evolved. Adding that test to core kind of limits the ability to evolve the implementation, since I think a committer would rightly be concerned about removing or altering a test case to coincide with a change in what should be perceived as an implementation change only.

To address this, my preferences in order would be:

1. Agree the hook weight implementation isn't something core should be testing, but open a new issue to discuss designing a supported API for doing this, outside of syncing entities (I realise this potentially should have been the purpose of THIS issue and syncing should have been spun out into another issue, with much narrower goals. FWIW, at one point the syncing approach was bundled with #3037136: Make Workspaces and Content Moderation work together, so I don't see why adding it in a dedicated issue with more detailed and dedicated test coverage should mean we suddenly need to boil the ocean on all possible approaches to the general problem.
2. Open a seperate issue to discuss and potentially add the hook weight test, but not block the current syncing approach.
3. Block the syncing fix behind agreeing on and implementing the hook weight test.

Sorry to be a stick in the mud, but if you still feel strongly about the test after reading these points I'll defer to your judgement.

I think another way of expressing my general concerns is: developers would be totally within their rights to stonewall changes in content moderations architecture in the future, based around their own assumptions and low level changes to the module, if we advertised replacing content moderation hooks in CRs/tests. I don't know what the scope of those assumptions might be, they might be surprising or incompatible with future features or architectural changes (just like cores treatment of pending revisions had to be reconsidered based on how AS chose to use them).

One completely anecdotal but real example is, I support a site that simply removes content moderations implementation of hook_entity_access. It's completely up to me to test and understand the impact of that and it's not something I expect to be tested, supported and catered towards in core.

@Sam152:

I still feel weird about writing and supporting a test that codifies implementation details of CM, I feel like it sends the wrong signals to implementors. [...]
Sorry to be a stick in the mud, but if you still feel strongly about the test after reading these points I'll defer to your judgement.

I don't feel as strongly as you do about this issue, but would you consider at least a "manual" test in your local env that messing with weights is actually a valid interim solution to use at own risk until a proper one is figured out? This would be enough to alleviate my concerns and I would be happy to move on with your preferred solution at that point:

Agree the hook weight implementation isn't something core should be testing, but open a new issue to discuss designing a supported API for doing this, outside of syncing entities

Sure, happy to do some manual testing, attached is the code I used to test this. I've also opened #3070517: Implement a solution for not forcing a new revision of non-syncing content in content moderation .

Added the following to the change record:

Note: the concept of a syncing content entity is still being explored and defined in core. You should carefully evaluate if your use-case fits the definition of a syncing content entity before using this API. This is being discussed in more detail here: #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities.

I think that addresses all the concerns raised, thanks again for following this up @plach :)

Thank you both for sticking with this :) I think we're ready to go back to RTBC now.

Saving credits

Committed 961c2b1 and pushed to 8.8.x. Thanks!

Thanks @plach, publishing change record.

Since this is a major bug fix which allows Content Moderation to be used together with Workspaces, I think we should backport it to 8.7.x as well.

Spoke with @xjm: we both feel that this change is potentially too disruptive to be backported to a patch release. People needing to use both CM and WS on 8.7.x may try to apply the committed patch (and deal with their hook_requirements() implementations).

Since SynchronizableEntityTrait::isSyncing already existed as an entity member, isn't it possible that some modules could already have been doing their own manipulation with this flag, and therefore this issue could change the behavior for them?

If so, this issue should probably have the CR updated to add that, as well as a release note added. (If I'm mistaken about this potential edgecase issue, the issue can be untagged and re-closed.)

Thanks!

Added a release note snippet with help from @Sam152.

Looks good, thanks!