Problem/Motivation

Symfony 4.2 has deprecated \Symfony\Component\Translation\TranslatorInterface::transChoice().

    /**
     * {@inheritdoc}
     *
     * @deprecated since Symfony 4.2, use the trans() method instead with a %count% parameter
     */
    public function transChoice($id, $number, array $parameters = [], $domain = null, $locale = null)
    {
        @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2, use the trans() one instead with a "%%count%%" parameter.', __METHOD__), \E_USER_DEPRECATED);

However we implement this method in \Drupal\Core\Validation\DrupalTranslator:

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

We did not get a deprecation notice as we do not call the parent method.

Steps to reproduce

Proposed resolution

Deprecate this method for removal, or just remove it entirely, as it is likely only called by upstream code.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#16 interdiff_13-16.txt895 bytesmurilohp
#16 drupal-trans-choice-deprecated-3255250-16.patch3.21 KBmurilohp
#13 interdiff_12-13.txt799 bytesmurilohp
#13 drupal-trans-choice-deprecated-3255250-13.patch3.13 KBmurilohp
#12 test-only-3255250-12.patch2.09 KBmurilohp
#12 interdiff_11-12.txt759 bytesmurilohp
#12 drupal-trans-choice-deprecated-3255250-12.patch3.08 KBmurilohp
#11 interdiff_5-11.txt3.02 KBmurilohp
#11 drupal-trans-choice-deprecated-3255250-11.patch3.08 KBmurilohp
#11 test-only-3255250-11.patch2.1 KBmurilohp
#5 drupal-trans-choice-deprecated-3255250-5.patch852 bytesmurilohp
#3 drupal-trans-choice-deprecated-3255250-3.patch1.83 KBmurilohp
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue tags: +Symfony 6
Parent issue: » #3161889: [META] Symfony 6 compatibility
murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Active » Needs review
FileSize
drupal-trans-choice-deprecated-3255250-3.patch1.83 KB

Hey @longwave, I searched the entire code on branch 10.0.x for transChoice and I haven't found anything, so following your proposed solution, removed the function.

Thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Needs work

I think it would be better to deprecate this, as contributed projects may still use it.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Needs work » Needs review
FileSize
drupal-trans-choice-deprecated-3255250-5.patch852 bytes

Thanks for the response, and I think it's a good catch, here's a new patch deprecating the method.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work
Issue tags: +Needs change record

We need 2 more things in the patch:

  1. The deprecation also needs to be added to the docblock of the method.
  2. We need testing for the deprecation message.

Outside of the patch: we need a change record.

@murilohp: Check the code for examples of how to do it.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Assigned: Unassigned » murilohp

Thanks for the feedback @daffie!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -Needs change record

Created a change record draft at https://www.drupal.org/node/3256859.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Also we can deprecate in 9.4.x and remove in Drupal 10 since Symfony 4.2 already deprecated the method and Drupal 10 will be based hopefully on Symfony 6.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Assigned: murilohp » Unassigned
Status: Needs work » Needs review
FileSize
test-only-3255250-11.patch2.1 KB
drupal-trans-choice-deprecated-3255250-11.patch3.08 KB
interdiff_5-11.txt3.02 KB

Thanks for the change record @Gábor Hojtsy, I followed your idea and changed the depreacation to D9.4, @daffie, I updated the docblock and about the test, I haven't found any tests for DrupalTranslation class, maybe we should open a new issue to cover this class. To cover the deprecation message, I created a new test class and a new function to validate this scenario.

So I'm uploading 2 patches, the first one is focused on D9.4 and it's the test-only. The second patch is for D9.4 and its the correct patch.
About the function cleanup(Drupal 10), we already have the patch #3.

Thanks!

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
FileSize
drupal-trans-choice-deprecated-3255250-12.patch3.08 KB
interdiff_11-12.txt759 bytes
test-only-3255250-12.patch2.09 KB

Uploading new patches to fix coding standards.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
FileSize
drupal-trans-choice-deprecated-3255250-13.patch3.13 KB
interdiff_12-13.txt799 bytes

Forgot the @see on #12(drupal-trans-choice-deprecated-3255250-12.patch). Now I think it's ready to go.

The last submitted patch, 12: test-only-3255250-12.patch, failed testing. View results

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

Patch looks good. Just one minor nitpick:

+++ b/core/tests/Drupal/KernelTests/Core/Validation/DrupalTranslatorTest.php
@@ -0,0 +1,58 @@
+    $translator->transChoice('There is one apple | There are @count apples', 1);

Every test need an assertion. Can you add one to this line. Something like the result is equal to: "There is one apple". The assertion does not need to be useful, it just need to be there.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Needs work » Needs review
FileSize
drupal-trans-choice-deprecated-3255250-16.patch3.21 KB
interdiff_13-16.txt895 bytes

@daffie here's a new patch asserting the transChoice call on the test. Since the string_translation is a mock object, I decided to assert just the class instance.

Thanks!

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

All the code changes look good to me.
The IS and the CR are in order.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed c274b2b8f3f to 10.0.x and 5b6ee132108 to 9.4.x. Thanks!

  • alexpott committed c274b2b on 10.0.x 
    Issue #3255250 by murilohp, daffie, longwave, Gábor Hojtsy: [Symfony 5]...

  • alexpott committed 5b6ee13 on 9.4.x 
    Issue #3255250 by murilohp, daffie, longwave, Gábor Hojtsy: [Symfony 5]...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Published the CR per #17