Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#36 | 2934026-36.patch | 11.76 KB | Sam152 |
#36 | interdiff.txt | 6.67 KB | Sam152 |
#32 | 2934026-32.patch | 9.49 KB | Sam152 |
#32 | interdiff.txt | 1.11 KB | Sam152 |
#27 | 2934026-27.patch | 9.49 KB | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSeeing what the deprecation listener picks up.
Comment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdding deprecation trigger_errors.
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCleaned 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.Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooking pretty good, I only found a couple of small points:
Not sure why we need to put this in an
if
condition, the entity type manager will always return a storage :)These deprecation messages need to reference 8.6.0 as well.
Comment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThe last implementation of this called off to
ModerationInformation::getLatestRevisionId
which handled falsey return values fromEntityTypeManagerInterface::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.I'll roll a new patch once the test bot tells us if we've caught all the method calls.
Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing feedback from #6, looks like all the usage of these methods were cleaned up.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #11
jofitz CreditAttribution: jofitz at ComputerMinds commentedReroll (attempt...)
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOne 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.Comment #14
BerdirThis 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?
Comment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGood 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.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTagging 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.
Comment #17
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commentedComment #19
vacho CreditAttribution: vacho at Skilld commentedSolving coding standard problem
Comment #20
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGreat, setting to "Needs review".
Comment #22
BerdirShould 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 :)
Comment #23
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSounds 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.
Comment #24
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReroll.
Comment #25
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReroll, 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:
Split out from
testDefaultAndLatestRevisionId
so it can be marked as @legacy in isolation and removed when the method is removed.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.
Comment #26
mikelutzShould 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'
Should read
More snuck in validator changes?
Needs method description, also missing @expectedDeprecation annotation to test for the deprecation error.
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.Comment #27
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@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?
Comment #28
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAlso, 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.Comment #29
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWhoops, forgot to update the CR in the test.
Comment #33
larowlanComment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI also spoke with @mikelutz in slack, I hope he doesn't mind being quoted here. Given the deadline for deprecations, the feedback was:
If anyone else is able to review this in the meantime, it would be a great one to get over the line before tomorrow.
Comment #35
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNit: I think we always include
()
at the end of the method names in our deprecation messages. Here we'd needgetLatestRevisionId()
for example.Same for all the others in the patch :)
Even though the replacement code is obvious, we should add two small methods in this test to also cover
::isLatestRevision()
and::getLatestRevision()
.This should also test for the expected deprecation message, like we do in
ModerationInformationTest::testGetLatestRevisionId()
.Comment #36
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAddressing feedback, thanks for the review :)
Comment #37
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooks great now, thanks!
Comment #38
alexpottCommitted and pushed 9f7df658e4 to 9.0.x and 244081f42c to 8.8.x and 4443296eed to 8.9.x. Thanks!