Properly deprecate \Drupal\Core\StringTranslation\TranslationWrapper

Two straightforward deprecations, no further deprecated code in this component.

I think we need to re-focus this issue to cause a proper deprecation for TranslationWrapper - since, fun fact via @Berdir, it's still present in our database dumps - see what happens if you do gunzip -c core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz | grep TranslationWrapper

So let's leave the rest of the deprecations here to #3104307: Remove BC layers in various Drupal\Core components and instead add an @trigger_error and derpecate this class for Drupal 10. And work out how to have an upgrade path for things like the our db dumps.

It was deprecated in CR https://www.drupal.org/node/2571255 before 8.0, would be great to understand how dumps were populated and why they having it - maybe re-build dump

We know why it does, it is part of entity type definitions for example, and no, we don't need to rebuild the dumps because real sites have it too, so our dumps having it is a good thing.

I'm not sure how to get rid of this to be honest. For installed entity type definitions, we can go through them and replace those classes with the undeprecated version. But who knows who else that stuff might be stored.

At least we need to deprecated it properly (guess 10.0.0 is ok here, then it needs 8.8 backport?) and add test

Fix copy/paste test method

Probably test should be extended to prove the class works

The better one

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -5,7 +5,19 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function __construct($string, array $arguments = [], array $options = [], TranslationInterface $string_translation = NULL) {
+    @trigger_error(__CLASS__ . ' is deprecated in drupal:8.0.0 and is removed from drupal:10.0.0. Use the \Drupal\Core\StringTranslation\TranslatableMarkup class instead. See https://www.drupal.org/node/2571255', E_USER_DEPRECATED);
+    parent::__construct($string, $arguments, $options, $string_translation);
+  }

this isn't a plugin - we can do the deprecation at the top of the class.

Thank you, updated IS and fixed #12

So the last patch is wrong because this update dump contains a lot of mentions in entity type definitions and

->values(array(
  'collection' => 'entity.update_backup',
  'name' => 'taxonomy_term.3192f1',

@andypost the last patch is correct. It shows that in order to deprecate TranslationWrapper we have to somehow fix the entries in the state tables as we're still using it.

@alexpott as I see in related dumps were created from 8-rc1 profile very probably on cache clear definitions are not updated

Probably it needs to upgrade dumps and write some update hook to check that definitions using to store TranslatableMarkup

This update works but needs tests and still fail of the test

Entity backups also could be fixed, stub as TODO

IMHO we should split up the deprecation change and getting rid of usages. Getting rid of the D9 deprecation is kinda beta-blocking, but once we change it to D10, it no longer is. So I'd propose to go with a minimal patch here.

That could be #11, or it could just be the existing description.

@Berdir #11 is not a good because adding a deprecation message to the constructor. I think what we should do here is to change the message to deprecate in D10 and file a follow-up to do #20.

I'm fine with either option, adding the message to the constructor or just the minimal part of changing the message. To me, adding it at least to the constructor allows us to warn on new uses of the class, where it's actually being instantiated. There aren't many of them but a few are still there: http://grep.xnddx.ru/search?text=new%20TranslationWrapper. And then we can move it outside when we eventually got rid of the serialized ones too.

And unless we do that by messing with the serialized strings, we'll need to load those objects so we can get rid of them, thereby triggering the deprecation. But then again, that's expected from update functions, that they sometimes need to trigger deprecated code. But it also means we'll need to those update functions into D9.x early enough to be able to remove them again if we want to have a chance for removing the class in D10.

I wanted to file follow-up to decide about clean-up, I stuck on finding more places where serialised classes could live in database

+1 to option where it will be less noisy because clean-up of serialisation could be tricky if we allow replacement for igbinary for example.

Moreover the trick with get/set maybe be not allowed for other places (outside of state)

@Berdir great point about the constructor deprecation and contrib. So yeah let's do #11 but we need a follow up to do #18 and move the deprecation to the class's MAIN section.

Filed follow-up for #18 #3115726: Clean-up usage of serialized TranslationWrapper from database

move the deprecation to the class's MAIN section.

I did not get it

For 11

#11 looks great. But it should be the last patch on the issue.

re-upload 11

@andypost thanks this looks great. I think we should also consider this for 8.9.x too.

Only concern with this issue is it seems like we can't actually remove it in 10.0.x for similar reasons to not being able to do so here - i.e. it's stored in the database, so until we resolve that it's not entirely deprecated.

Maybe just a follow-up to discuss that issue a bit though?

@catch I filled follow up for that in #25

Oh good point. In that case.

Committed d185342 and pushed to 9.0.x. Thanks!