[Symfony 6] Revert 3231603 to use our own TranslatorInterface

This piece of core looks pretty weird tbh, see https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/lib/Drupal/...

DrupalTranslator implements TranslatorInterface, but TranslatorInterface has only methods (trans and getLocale), but comments state opposite, for example:

/**
   * {@inheritdoc}
   */
  public function transChoice($id, $number, array $parameters = [], $domain = NULL, $locale = NULL) {

/**
   * {@inheritdoc}
   */
  public function setLocale($locale) {
    $this->locale = $locale;
  }

Seems like this is old code, that is not used anywhere.

Then, more to add, DrupalTranslator itself is not widely used. Actually, I have found only core usage and single contrib module, according to this: http://grep.xnddx.ru/search?text=DrupalTranslator&filename=

Thus, it might be changed more dramatically between 9 and 10.

this is a very interesting topic, as currently Symfony\Contracts\Translation\TranslatorInterface comes not from regular Symfony packages, but on contracts ones, in particular in D9 it comes from symfony/translation-contracts and has version v2.5.0 (with my live 9.3.0 project) and it already do not have ::transChoice(), so it's like that for a long time and it probably can be just safely removed.

See https://github.com/symfony/translation-contracts

Probably, in D10 those contracts packages should be updated to version ^3

$ composer info "symfony/*" | grep contracts                                                                                     
symfony/cache-contracts            v2.5.0  Generic abstractions related to caching
symfony/deprecation-contracts      v2.5.0  A generic function and convention to trigger deprecation notices
symfony/event-dispatcher-contracts v1.1.11 Generic abstractions related to dispatching event
symfony/http-client-contracts      v2.5.0  Generic abstractions related to HTTP clients
symfony/service-contracts          v2.5.0  Generic abstractions related to writing services
symfony/translation-contracts      v2.5.0  Generic abstractions related to translation

Drupal\Tests\comment\Functional\CommentAnonymousTest::testAnonymous()

Maybe it's just unlocked some issue in comment module?

Hey @longwave, this is just a suggestion, but if we follow the approach of removing symfony/translation, what do you think about typehint our own interface to return a MarkupInterface instead of string(patch #4, method trans).

The following review is from the patch #4, and I'm assuming we're going to keep the idea of type hinting the class.

+++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
@@ -74,7 +74,7 @@ public function setLocale($locale) {
     return $this->locale ? $this->locale : \Drupal::languageManager()->getCurrentLanguage()->getId();

What do you think about removing the unnecessary $this->locale here? I mean use something like:
return $this->locale ?? \Drupal::languageManager()->getCurrentLanguage()->getId();

Reverting #3231683: [Symfony 6] A number of methods of the class Drupal\Core\TypedData\Validation\ExecutionContext are considered internal and Drupal should not override them. and doing mostly the same thing as Symfony, only a little bit different does not make me very happy. To me, we should see if we can do the same thing as Symfony. Drupal is a Symfony app.

Bumping this to critical as this issue is blocking Symfony 6 in D10.

Working on this.

The patch is the same as the patch from comment #4, only with failures fixed.

Edit: The change from variables with %value to @value is necessary because variables that start with an "%" will get '' added before the value and '' added at the end. Variables that start with "@" do not get that. The required return type hint by Symfony make it so that less is possible. The problem is only for validation contstraint violations messages. To me, this is something we shall have to learn to accept. Not that I am happy about it.

Hey @daffie I liked your solution, it's the original approach and I think we should follow the idea of doing the same thing as Symfony.

About the code:

+++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
@@ -74,8 +74,8 @@ public function setLocale($locale) {
+    return $this->locale ?? \Drupal::languageManager()->getCurrentLanguage()->getId();

Thanks! For the change!

+1 for me to RTBC this!

Hm, is that the only way to go for validation constraints? That sounds like a stepping stone that could lead to more things that we need to change in the future.

I've opened https://github.com/symfony/symfony/pull/44911 so we can see what Symfony thinks about extending the scope.

Given the direction that https://github.com/symfony/symfony/pull/44911 is taking (going nowhere), I think we should do #3255245-14: [Symfony 6] Revert 3231603 to use our own TranslatorInterface here.

Then we should open a follow-up to track whether there's an alternative approach, and also revisit #3054535: Discuss whether to decouple from Symfony Validator again.

That makes sense. Let's send in #14 again for another round then :)

Looks like #3255250: [Symfony 5] TranslatorInterface::transChoice() is deprecated doesn't like the way this issue is evolving...

Ouch, but looking at #3255250-3: [Symfony 5] TranslatorInterface::transChoice() is deprecated this approach might be better anyway.

The fails are all seem to be due to the transChoice() method missing, that the patch removes :) I don't think we should remove it, just not deprecate it like the rest of the things we are not deprecating?

Thanks @Gábor Hojtsy, you're right, looks like undeprecating (my English spellchecker is hating me even more today...) is the way to go.

Let's see if I got it right this time

Updating the title and the proposed resolution from the IS with the comment #12

Added a release notes snippet as well and further expanded the issue summary.

All code changes look good to me.
The patch is reverting the patch from #3231603: Allow reducing scope for selected modules.
The problem from that issue now need to be fixed in another way. I have created #3258380: [Symfony 6][Second try] A number of methods of the class Drupal\Core\TypedData\Validation\ExecutionContext are considered internal and Drupal should not override them. for that problem.
I have linked this issue to the CR from the issue we are reverting. That CR needs to be unpublished.
For me it is RTBC.

Committed d28e89b and pushed to 10.0.x. Thanks!
Committed 1201aa8 and pushed to 9.4.x. Thanks!
Committed 4bb9f46 and pushed to 9.3.x. Thanks!

I fixed the conflict in 9.3.x core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php - hopefully tests won't fail but if they do - I'll hotfix.

Thanks for updating the change record, I further updated it to make the undeprecation more visual and updated the title: https://www.drupal.org/node/3238432 Should we have a dedicated change record for the undeprecation?

Should we have a dedicated change record for the undeprecation?

No, I do not think so. Or did we do an official release with Drupal with original patch in it?

Yes our deprecation was released in 9.3.0.

Spot checked some methods from the CR:

http://grep.xnddx.ru/search?text=markConstraintAsValidated&filename= 0 references

http://grep.xnddx.ru/search?text=setConstraint%28&filename= - 1 reference in a test

http://grep.xnddx.ru/search?text=isGroupValidated%28&filename= - 0 references

http://grep.xnddx.ru/search?text=isConstraintValidated%28&filename= - references

Let's leave it as is :)