Problem/Motivation

Core now provides an API for these methods natively since #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision.

Proposed resolution

Deprecate these methods from the moderation information service, so CM has less code, tests and API surface to maintain!

Remaining tasks

Commit!

User interface changes

None.

API changes

3 new deprecations.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
3.57 KB

Seeing what the deprecation listener picks up.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sam152’s picture

Adding deprecation trigger_errors.

Sam152’s picture

Cleaned up the usages of the deprecated methods that storm could find. I marked the ModerationInformation tests as @group legacy, didn't want to touch any of the test coverage for these while the methods were being altered to use the new core API, but they should essentially be already tested at this point.

amateescu’s picture

Looking pretty good, I only found a couple of small points:

  1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -74,8 +74,10 @@ public function shouldModerateEntitiesOfBundle(EntityTypeInterface $entity_type,
    +    if ($storage = $this->entityTypeManager->getStorage($entity_type_id)) {
    

    Not sure why we need to put this in an if condition, the entity type manager will always return a storage :)

  2. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
    @@ -58,6 +58,8 @@ public function shouldModerateEntitiesOfBundle(EntityTypeInterface $entity_type,
    +   * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
    
    @@ -72,6 +74,8 @@ public function getLatestRevision($entity_type_id, $entity_id);
    +   * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
    
    @@ -122,6 +126,8 @@ public function isPendingRevisionAllowed(ContentEntityInterface $entity);
    +   * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
    

    These deprecation messages need to reference 8.6.0 as well.

The last submitted patch, 4: 2934026-4.patch, failed testing. View results

Sam152’s picture

  1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -74,8 +74,10 @@ public function shouldModerateEntitiesOfBundle(EntityTypeInterface $entity_type,
       public function getLatestRevision($entity_type_id, $entity_id) {
    -    if ($latest_revision_id = $this->getLatestRevisionId($entity_type_id, $entity_id)) {
    -      return $this->entityTypeManager->getStorage($entity_type_id)->loadRevision($latest_revision_id);
    +    @trigger_error('The ' . __METHOD__ . ' method is deprecated since version 8.6.x and will be removed before 9.0.0.', E_USER_DEPRECATED);
    +    /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
    +    if ($storage = $this->entityTypeManager->getStorage($entity_type_id)) {
    +      return $storage->loadRevision($storage->getLatestRevisionId($entity_id));
         }
       }
    

    The last implementation of this called off to ModerationInformation::getLatestRevisionId which handled falsey return values from EntityTypeManagerInterface::getStorage. I was trying to carry forward the exact same semantics in each method from before and after the deprecation. It does however seem like in this instance, it's totally redundant.

    Lets not introduce new code which does this incorrectly, as you suggest, but lets try not clean up the other incorrect usages of getStorage just yet. We can open a follow up to review across all of content_moderation or core.

  2. Good catch!

I'll roll a new patch once the test bot tells us if we've caught all the method calls.

Sam152’s picture

Fixing feedback from #6, looks like all the usage of these methods were cleaned up.

Sam152’s picture

Issue tags: +Needs reroll
jofitz’s picture

Issue tags: -Needs reroll
FileSize
6.59 KB

Reroll (attempt...)

Sam152’s picture

One last change, I've since learned the @legacy annotation can be added to individual test methods. I think it makes sense to do that here in all the test cases. Otherwise, this looks good to me.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

+++ b/core/modules/content_moderation/tests/src/Kernel/NodeAccessTest.php
@@ -67,6 +67,8 @@ protected function setUp() {
   /**
    * Tests for moderation information methods with node access.
+   *
+   * @group legacy
    */
   public function testModerationInformation() {

This is the only test method in this class, if it's really the only purpose of this to test the old methods (and we have equivalent test coverage for the methods on entity storage) then we should add @legacy on the whole test as we'd remove it completely in 9.x?

If we don't have that test coverage, then maybe it should instead be converted to use the new methods?

Sam152’s picture

Good catch. In both of the tests in the legacy group, we are testing a mixture of deprecated and non-deprecated methods. I've updated the patch so that test methods marked legacy are only testing the deprecated methods and nothing else.

While these methods have the equivalent tests in core and you could thus argue they can be removed, keeping them ensures the deprecations listener is tracking of them, so we have a pointer to go and remove them in the future. If convention is to delete tests as things are deprecated, happy to remove them.

Sam152’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Tagging for novice. To move this issue forward, we need to update the version numbers in the deprecation messages. I think the window to deprecate this for 8.7 is probably passed, so they would be deprecated in 8.8.

Both the deprecation messages and the expected deprecation messages in the tests should be changed.

govind.maloo’s picture

Status: Needs work » Needs review
FileSize
9.16 KB
4.53 KB

Status: Needs review » Needs work

The last submitted patch, 17: 2934026-17.patch, failed testing. View results

vacho’s picture

Solving coding standard problem

Sam152’s picture

Status: Needs work » Needs review

Great, setting to "Needs review".

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Should we deprecate ->isLiveRevision() as well? It is completely unused in drupal core, there aren't even any test for it. I had to remove several calls to it in the moderation_sidebar module because it simply didn't make any sense :)

Sam152’s picture

Sounds like a reasonable task, but lets do it in another issue since there isn't a 1 to 1 replacement in core. It might need it's own CR trying to describe the original intentions of the method and why it no longer makes sense.

Sam152’s picture

Sam152’s picture

Issue tags: -Novice
FileSize
9.42 KB

Reroll, not sure how the validator changes snuck into the last patch. Also did a self review and annotated some of the less obvious changes here:

  1. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationInformationTest.php
    @@ -77,6 +76,22 @@ public function testDefaultAndLatestRevisionId() {
    +  /**
    +   * @covers ::getLatestRevisionId
    +   * @group legacy
    +   */
    +  public function testGetLatestRevisionId() {
    +    $entity_test_rev = EntityTestRev::create([
    +      'name' => 'Default Revision',
    +      'moderation_state' => 'published',
    +    ]);
    +    $entity_test_rev->save();
    +
    +    $entity_test_rev->name = 'Pending revision';
    +    $entity_test_rev->moderation_state = 'draft';
    +    $entity_test_rev->save();
    

    Split out from testDefaultAndLatestRevisionId so it can be marked as @legacy in isolation and removed when the method is removed.

  2. +++ b/core/modules/content_moderation/tests/src/Kernel/NodeAccessTest.php
    @@ -77,12 +77,29 @@ public function testModerationInformation() {
    +  /**
    +   * @covers \Drupal\content_moderation\ModerationInformation::getLatestRevisionId
    +   * @group legacy
    +   */
    +  public function testGetLatestRevisionId() {
    +    // Create an admin user.
    +    $user = $this->createUser([], NULL, TRUE);
    +    \Drupal::currentUser()->setAccount($user);
    +
    +    // Create a node.
    +    $node = $this->createNode(['type' => 'page']);
    +    $this->assertEquals($node->getRevisionId(), $this->moderationInformation->getLatestRevisionId('node', $node->id()));
    +
    +    // Create a non-admin user.
    +    $user = $this->createUser();
    +    \Drupal::currentUser()->setAccount($user);
         $this->assertEquals($node->getRevisionId(), $this->moderationInformation->getLatestRevisionId('node', $node->id()));
       }
    

    This part continues to test the access agnostic aspect of moderation information, but is pulled out of the main testModerationInformation method, so it can be easily marked as @legacy and removed when the method is removed.

I think this should be ready to go if it passes.

mikelutz’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -86,26 +86,20 @@ public function shouldModerateEntitiesOfBundle(EntityTypeInterface $entity_type,
    +    @trigger_error('The ' . __METHOD__ . ' method is deprecated since version 8.8.x and will be removed before 9.0.0.', E_USER_DEPRECATED);
    ...
       /**
    ...
    +    @trigger_error('The ' . __METHOD__ . ' method is deprecated since version 8.8.x and will be removed before 9.0.0.', E_USER_DEPRECATED);
    
    @@ -143,7 +137,8 @@ public function getAffectedRevisionTranslation(ContentEntityInterface $entity) {
    +    @trigger_error('The ' . __METHOD__ . ' method is deprecated since version 8.8.x and will be removed before 9.0.0.', E_USER_DEPRECATED);
    

    Should read __METHOD__ . 'is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use RevisionableStorageInterface::<replacement> instead. See https://www.drupal.org/node/2927226'

  2. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
    @@ -69,6 +69,8 @@ public function isModeratedEntityType(EntityTypeInterface $entity_type);
    +   * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
    
    @@ -83,6 +85,8 @@ public function getLatestRevision($entity_type_id, $entity_id);
    +   * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
    
    @@ -120,6 +124,8 @@ public function getAffectedRevisionTranslation(ContentEntityInterface $entity);
    +   * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
    

    Should read

    @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.  Use RevisionableStorageInterface::<replacement> instead.
    @see https://www.drupal.org/node/2927226
    
  3. +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php
    @@ -4,6 +4,8 @@
    +use Drupal\Core\Entity\ContentEntityInterface;
    +use Drupal\Core\Entity\EntityInterface;
    
    +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationInformationTest.php
    @@ -77,6 +76,22 @@ public function testDefaultAndLatestRevisionId() {
     
    

    More snuck in validator changes?

  4. +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationInformationTest.php
    @@ -77,6 +76,22 @@ public function testDefaultAndLatestRevisionId() {
    +  /**
    +   * @covers ::getLatestRevisionId
    +   * @group legacy
    +   */
    

    Needs method description, also missing @expectedDeprecation annotation to test for the deprecation error.

  5. +++ b/core/modules/content_moderation/tests/src/Kernel/NodeAccessTest.php
    @@ -77,12 +77,29 @@ public function testModerationInformation() {
    +    $this->assertEquals($node->getRevisionId(), $this->moderationInformation->getLatestRevisionId('node', $node->id()));
    

    Should be assertSame

The ideal way to set up the legacy tests in this type of situation is to take the main test file, and update it as needed to test the new best practice functionality. Then make a new LegacyModerationInformationTest class to test the three deprecated methods. Mark the class @legacy, each method should set up a testable situation, have an @expectedDeprecation annotation, and essentially ->assertSame(<current way>, <legacy way>) The reason for the new file is that it is easier for some of our static analysis tools to ignore deprecated method calls if they are in a test file that starts with the word "Legacy", and it is easier to find and delete those files than it will be to delete individual @group legacy methods. The reason for the single assertion is to show that the legacy method result matches the new method result (which it should, as it should be calling the new method internally anyway) The existing tests work to test all the actual new logic, the legacy tests are primarily to ensure the deprecation error is being triggered, and a sanity check to make sure they do the same thing as the new method.

Sam152’s picture

Status: Needs work » Needs review
FileSize
6.13 KB
9.49 KB

@mikelutz thank you for the excellent review.

1,2, I didn't realise there was a consistent format for these messages, I've updated them all.
3, Good catch, no idea why that keeps happening.
4, Done.
5, I believe this is a seperate issue tracked and reported here: #2938550: getLoadedRevisionId() doesn't return an integer, revision IDs are string in some places an integers in others.
6, I think since in this case the replacement is in core, duplicating the test and using the new methods would be duplicating testing in core, I don't think CM should directly test those methods (even though there is probably a tonne of implicit tests!). Hopefully this restructure isn't a blocker?

Sam152’s picture

Also, I added a dedicated change record for this issue. I'm going to update that with examples of before/afters using the moderation_information service, so users should have an easy find/replace instead of understanding the CR that introduced the replacements.

Sam152’s picture

Issue summary: View changes

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Status: Needs review » Needs work

The last submitted patch, 27: 2934026-27.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
9.49 KB

Whoops, forgot to update the CR in the test.

larowlan’s picture

Sam152’s picture

I also spoke with @mikelutz in slack, I hope he doesn't mind being quoted here. Given the deadline for deprecations, the feedback was:

Patch looks good at a glance, I'll review tomorrow and rtbc. Test stuff shouldn't be a blocker. You are right, cm doesn't need to test the revisionable interface stuff directly, but the legacy tests should prove that the deprecated methods still work.

If anyone else is able to review this in the meantime, it would be a great one to get over the line before tomorrow.

amateescu’s picture

  1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -86,26 +86,20 @@ public function shouldModerateEntitiesOfBundle(EntityTypeInterface $entity_type,
    +    @trigger_error(__METHOD__ . ' is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use RevisionableStorageInterface::getLatestRevisionId and RevisionableStorageInterface::loadRevision instead. See https://www.drupal.org/node/3087295', E_USER_DEPRECATED);
    

    Nit: I think we always include () at the end of the method names in our deprecation messages. Here we'd need getLatestRevisionId() for example.

    Same for all the others in the patch :)

  2. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
    --- a/core/modules/content_moderation/tests/src/Kernel/ModerationInformationTest.php
    +++ b/core/modules/content_moderation/tests/src/Kernel/ModerationInformationTest.php
    

    Even though the replacement code is obvious, we should add two small methods in this test to also cover ::isLatestRevision() and ::getLatestRevision().

  3. +++ b/core/modules/content_moderation/tests/src/Kernel/NodeAccessTest.php
    @@ -77,12 +77,29 @@ public function testModerationInformation() {
    +   * @covers \Drupal\content_moderation\ModerationInformation::getLatestRevisionId
    +   * @group legacy
    ...
    +  public function testGetLatestRevisionId() {
    

    This should also test for the expected deprecation message, like we do in ModerationInformationTest::testGetLatestRevisionId().

Sam152’s picture

Addressing feedback, thanks for the review :)

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +@deprecated

Looks great now, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 9f7df658e4 to 9.0.x and 244081f42c to 8.8.x and 4443296eed to 8.9.x. Thanks!

  • alexpott committed 9f7df65 on 9.0.x
    Issue #2934026 by Sam152, govind.maloo, vacho, Jo Fitzgerald, amateescu...

  • alexpott committed 244081f on 8.8.x
    Issue #2934026 by Sam152, govind.maloo, vacho, Jo Fitzgerald, amateescu...

  • alexpott committed 4443296 on 8.9.x
    Issue #2934026 by Sam152, govind.maloo, vacho, Jo Fitzgerald, amateescu...

Status: Fixed » Closed (fixed)

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