Deprecate EntityRevisionConverter in content_moderation.

@Sam152 in #2865616-77: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option.:

Paramconverters, access checkers, event subscribers and similar services which are never expected to be used directly either as services or value objects, are not considered part of the API.

Of course we can argue such things in the follow up :)

Well, my goal was reducing disruption (https://www.drupal.org/core/d8-allowed-changes#beta :), given that EntityRevisionConverter is itself an example of a param converter extending another one. OTOH this is not considered a best practice so removing it could be fine:

In general, only interfaces can be relied on as the public API.
With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do.

Yep, that's exactly what I thought, sorry for not being clear.

It seems #7 is only the actual patch, not the combined one.

The parent issue is in, let's test the patch above again.

Looks great to me

Hm, not sure about adding this message to the suppressed deprecation messages. It makes sense to temporarily suppress the warning for something like drupal_set_message(), but I'm not sure it does for something like this that has low usage. Still trying to follow the implications of #15.

If we do add it to the message list I think there should be an inline comment above it, with the reasoning, and a link to any followup.

I wonder if a "soft" deprecation could be an option. As @Sam152 mentioned, the classes are already marked @internal so we could remove them with no issues? Maybe we could add a message in the comments to say "It's advised to use the "paramconverter.entity" service instead, which now supports loading the latest revision."

1. +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
   @@ -2,104 +2,28 @@
   + *   load_pending_revision flag. The core entity converter now natively loads
   

   This could mention that load_pending_revision is the predecessor of load_latest_revision

2. +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
   @@ -2,104 +2,28 @@
   + *   the latest revision of an entity for entity forms and when the
   

   s/and when/when/

3. +++ b/core/modules/content_moderation/tests/src/Kernel/EntityRevisionConverterTest.php
   @@ -2,142 +2,63 @@
   + * @group legacy
   ...
   +   * @expectedDeprecationMessage The load_pending_revision flag has been deprecated. You should use load_latest_revision instead.
   

   👏

Per #2860097-100: Ensure that content translations can be moderated independently.2, this would make that issue slightly simpler.

And actually, I think this patch looks great. #23 are both super nits, really. So … RTBC :)

This patch looks great to me, and since content_moderation is still an experimental module (until #2897132: Mark Content Moderation module as stable lands), I'm happy to cherry pick this to 8.5.x as well, so setting the Version accordingly.

Just one question about the test coverage:

+++ b/core/modules/content_moderation/tests/src/Kernel/EntityRevisionConverterTest.php
@@ -2,142 +2,63 @@
-  public function testConvertNonRevisionableEntityType() {
-  public function testConvertNoEditFormHandler() {

Is there a reason we're removing these tests entirely? Should we instead move them to EntityConverterLatestRevisionTest or elsewhere?

Looks good to me, the only concern I have is that the route defined in EntityResolverManagerTest::testSetLatestRevisionFlag() is /foo/{entity_test} rather than /foo/{entity_test_rev}. Not sure that's relevant.

Is there a reason we're removing these tests entirely?

I asked myself this very question, and first included it in #23. But then I realized that the "edit form" thing was a hardcoded assumption/restriction in the Content Moderation param converter because it was meant to be a param converter used only by/for Content Moderation. Now that that param converter has been generalized into something more broadly useful, there also is no more need for that restriction, because nothing should be using the Content Moderation param converter. Hence also the explicit deprecation error.

The only possible edge case that could regress here is that if you have a route that has an entity edit form, and does not have load_pending_revision set, then this paramconverter would have worked before (applies() would have returned TRUE), and now you'd have to manually set the load_latest_revision flag.

It just seemed so far-fetched that I figured it was safe to go ahead. Now I'd like somebody to explicitly confirm that this edge case is not a problem.

On quick inspection, #26 looks like adequate test coverage to me, but I haven't yet dug into the background of why these tests were needed to begin with, so I'm not yet 100% clear on whether the new tests address all of the same original concerns. @Sam152, @plach, @Wim Leers, @timmillwood: what do you all think?

I agree with #30 that the test coverage looks adequate, although it never feels right to delete test.

It also looks like #27 still needs addressing?

Ticking credit boxes for reviewers.

#26 no longer applies. This is just a straight reroll.

Given that this was already RTBC in #24, and that more test coverage was added since then, I decided to commit this without awaiting resolution on #32, in order to allow #2860097: Ensure that content translations can be moderated independently to be simplified.

Marking this Fixed, but adding the Needs followup tag for #32. If an additional test is needed, please open a new issue for it. If it turns out not to be needed, please remove the tag.

Thanks!