Fix deprecation messages related to deprecated comment Action plugins

Hi @mairi thanks for working on this. These deprecations are triggered by out update tests. We need to add the @group legacy to all concrete (ie. not abstract) tests that extend \Drupal\FunctionalTests\Update\UpdatePathTestBase and then we should be able to make some headway in removing these.

in the end I modified every concrete class.

I will clear the last 15

I want to work in small tightly focused steps.

These next changes are ones which I can clearly see are one level away from extending UpdatePathTestBase

It does not fix all the issues...but hey that is the nature of small but justifiable concrete steps.

I would appreciate some advice. The remaining errors share a common thread. [ namely the use of UpdatePathTestBase::runUpdates(). ]

UpdatePathTestBase is a abstract class and so subject to the maxim from #11 about not adding @legacy tags.

What is the solution here ????

Taking FileUpdateTest as a example of the problem ... should it be rewritten so that it no longer extends UpdatePathTestBase.. that would solve the problem ... I guess but maybe that is for another issue?

In minor news .. the patch here just scoops up a single failing test that should have been included in #16

@martin107 - the problem is that the failing tests are extending \Drupal\simpletest\WebTestBase we need to convert them to BrowserTestBase test. There are probably existing issues for this.

So WebTestBase tests don't support multiple groups - there is an issue but we're unlikely to do it because webtestbase are on the way out.

Here's a fix to make WtB respect the @group legacy.

Thanks for the expert touch .. that is really appreciated.

The patch is coding standard violation free... and with hindsight that is a really good solution

So I think this patch is ready to commit....

alexpott++

The only relevante code-change for this issue is in WebTestBase.
That change seems to make a lot of sense.

It's likely we can remove more skipped deprecations here. At the very least we should remove all the obviously related...
ie

      'Drupal\node\Plugin\Action\PublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\PublishAction instead. See https://www.drupal.org/node/2919303.',
      'Drupal\node\Plugin\Action\SaveNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\SaveAction instead. See https://www.drupal.org/node/2919303.',
      'Drupal\node\Plugin\Action\UnpublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead. See https://www.drupal.org/node/2919303.',

and

      'Drupal\node\Plugin\Action\DeleteNode is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.',
      'Drupal\comment\Plugin\Action\DeleteComment is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.',

LGTM

Re-rolled... let's find out if any new update tests have been added since 31st May.

Nice so doing this shows how necessary this is to keep all the tests honest. It's caught the fact that the config used in the install from existing config is out-of-date.

The configuration had action plugins configured with the old IDs.

Fixes deprecation issues in #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides.

Looks good!

I forgot to mention: I did diff the tarballs: uuids and config hashes have changed in testing_config_install.tar.gz, while they did not in multilingual.tar.gz, but anyway looks fine.

Find the diffs attached.

Committed 163144a and pushed to 8.6.x. Thanks!

Where's the CR saying that you have to use @group legacy on any UpdatePathTestBase subclass?

The reason these updates tests trigger the deprecation warnings is because we're doing this in a post update hook using entity API and not modifying the raw configuration values. See some of the motivation behind that here. It illustrates an interesting point however, if the deprecated plugin classes were removed, won't we break the continuity of the update hooks and 9.0.0 would essentially require system_post_update_change_action_plugins to already have run?

Realistically it probably wont be an issue in this case, but it might be worth documenting that adding @legacy to an update test means:

• The test will likely hard-fail when the deprecated code is deleted.
• The minimum schema version for an update to the next major version becomes > than the failing hook.

I wonder if a better fix here would be to revisit using the configuration API instead? The code could still live in post-update, given we wouldn't want to mess with any sites that had already run these hooks.

I really don't care why something is broken in this case.

I just know that before this commit, I could write update path tests, and now those tests fail. And after debugging, everything points to this issue. But it would have been nice for a CR to explain why everything went sideways

Created a CR - https://www.drupal.org/node/2985785 and filed a followup issue to add documentation to \Drupal\FunctionalTests\Update\UpdatePathTestBase. Sorry for not doing this.

Much appreciated, thanks.

Future idea for a follow-up: magically opt all @group Update into the legacy group behind the scenes...