Replace usage of the deprecated LOCALE_PLURAL_DELIMITER constant

#Nashville2018, #Novice
It would be helpful with a "how to review" instruction in this issue.
Is it enough to apply the patch and check that the installation appears to be working? How should it be tested/reviewed?

Patch does not applies to 8.6.x branch. Needs reroll.

Here is updated patch for 8.6.x. I have also reverted the reordering of use statements from patch #4 because this is not in scope of this issue. If reordering of use statement is necessary then it should be done in separate issue otherwise this will create scop creep.

As mentioned in #4 the use of LOCALE_PLURAL_DELIMITER constant in Drupal\Component\Gettext\PoItem component. That should be removed in #1637662: Clean up the Gettext PO parser

Here is the rerolled patch for 8.7.x.

@msankhala:

Thanks for continuing to work on this!

Good point in #12 about reordering the use statements. That should be fixed, but not as part of this issue.

I reviewed the patch, making sure that it correctly spells the old and new constants, that every file gets a use statement, and so on. (Not quite: NumericField.php already had the use statement.) It mostly looks good. One little problem:

--- a/core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php
+++ b/core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php
@@ -4,8 +4,10 @@
 
 use Drupal\Component\Gettext\PoHeader;
 use Drupal\file\Entity\File;
+use Drupal\Core\StringTranslation\PluralTranslatableMarkup;
 use Drupal\Tests\views\Functional\ViewTestBase;
 
+
 /**
  * Tests the creation of numeric fields.
  *

Why add a blank line?

I also searched for LOCALE_PLURAL_DELIMITER in the code base after applying the patch:

$ grep -ril LOCALE_PLURAL_DELIMITER core
core/includes/common.inc
core/lib/Drupal/Component/Gettext/PoMemoryWriter.php
core/lib/Drupal/Component/Gettext/PoItem.php

We need to keep the reference in common.inc (with the deprecation notice).

We need to get rid of the references in the Gettext component. The only question is whether we should do it as part of this issue.

I think the simplest solution is to define a class constant DELIMITER (or maybe PLURAL_DELIMITER) in the PoItem class. Then use it in both classes. They are in the same namespace, so no use statement is needed. I see that, in PoMemoryWriter, LOCALE_PLURAL_DELIMITER appears only twice, in a function that has a PoItem parameter. That means the PoMemoryWriter class already "knows about" the PoItem class.

If you prefer not to make that change as part of this issue, then please create a follow-up issue to handle it. If you do, then I might postpone this issue on the "follow-up". Either way, please remove the stray blank line I mentioned above.

One other thing:

--- a/core/modules/system/system.module
+++ b/core/modules/system/system.module
@@ -27,6 +27,7 @@
 use Drupal\user\UserInterface;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use GuzzleHttp\Exception\RequestException;
+use Drupal\Core\StringTranslation\PluralTranslatableMarkup;

We agree that alphabetizing all the use statements in system.module is out of scope for this issue, but let's not make it worse than it is. Why not add the line between these two?

use Drupal\Core\PhpStorage\PhpStorageFactory;
use Drupal\Core\Routing\RouteMatchInterface;

Same point in NumericFormatPluralTest.php. I already quoted the relevant part (hunk) of the patch in my previous comment.

@benjifisher have removed the stray blank line and added the use statement at the place you suggested.

Regarding the changes in the PoItem and PoMemoryWriter should a follow-up issue IMO.

@imalabya:

Thanks, the two changes that you made look good.

In #16, I wrote.

Same point in NumericFormatPluralTest.php. I already quoted the relevant part (hunk) of the patch in my previous comment.

Referring to the patch in #17, the relevant lines are

--- a/core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php
+++ b/core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php
@@ -4,6 +4,7 @@
 
 use Drupal\Component\Gettext\PoHeader;
 use Drupal\file\Entity\File;
+use Drupal\Core\StringTranslation\PluralTranslatableMarkup;
 use Drupal\Tests\views\Functional\ViewTestBase;

Again, changing the order of existing use statements is out of scope for this issue, but we should try not to make things worse with the lines we add. In this case, that means we should preserver the (case-insensitive) alphabetical order.

In #15, I wrote,

If you prefer not to make that change as part of this issue, then please create a follow-up issue to handle it.

referring to the changes to the Gettext component. In #17, you said that these changes should be in a follow-up issue, but you did not create that issue.

@benjifisher I've applied the changes requested in #19 and #15:

-I put the 'use' statements in NumericFormatPluralTest.php into alphabetical order.
-I created a class constant in the PoItem class.
-I replaced the two instances of LOCALE_PLURAL_DELIMITER in PoMemoryWriter.php
-I replaced the three instances of LOCALE_PLURAL_DELIMITER in PoItem.php

For the comment on the class constant in the PoItem class, I followed the lead of the PERMANENT constant in the Cache class in Drupal\Core\Cache and just repeated the first part of the comment on the DELIMITER constant in PluralTranslatableMarkup.

Happy to make further adjustments if needed.

Please follow this patch up.

Grep shows that Components "poisoned" by common.inc constant still - looks it needs follow-up to fix

core8$ git grep LOCALE_PLURAL_DELIMITER
core/includes/common.inc:133:const LOCALE_PLURAL_DELIMITER = PluralTranslatableMarkup::DELIMITER;
core/lib/Drupal/Component/Gettext/PoItem.php:189:        strpos($this->source, LOCALE_PLURAL_DELIMITER) !== FALSE) {
core/lib/Drupal/Component/Gettext/PoItem.php:190:      $this->setSource(explode(LOCALE_PLURAL_DELIMITER, $this->source));
core/lib/Drupal/Component/Gettext/PoItem.php:191:      $this->setTranslation(explode(LOCALE_PLURAL_DELIMITER, $this->translation));
core/lib/Drupal/Component/Gettext/PoMemoryWriter.php:29:      $item->setSource(implode(LOCALE_PLURAL_DELIMITER, $item->getSource()));
core/lib/Drupal/Component/Gettext/PoMemoryWriter.php:30:      $item->setTranslation(implode(LOCALE_PLURAL_DELIMITER, $item->getTranslation()));

reroll because of system.module's hunk

In this issue we should replace all deprecated usage including the component

Digging more related issues it becomes clear that here it needs to move definition of delimiter to Gettext component

Core using delimiter in following places
- .po files reader/writer - conditionally flipping source and translation depending on number of plurals - #2918489: Plurals are not exported correctly when exporting source translations
- storing plurals in config - also using flip/flop on export and to create translations- #2545730: Misuse of formatPlural() in Numeric field prefix/suffix
- UI elements and controls to edit translations

So I went ahead and using a patch from #1637662: Clean up the Gettext PO parser
Which deprecates \Drupal\Core\StringTranslation\PluralTranslatableMarkup::DELIMITER for \Drupal\Component\Gettext\PoItem::DELIMITER it needs change record

PS: no interdiff because it bigger then patch

One more leftover

I applied the patch in #27 and ran "grep -ril LOCALE_PLURAL_DELIMITER core" from the root of my test installation. The command only returned the single instance of LOCALE_PLURAL_DELIMITER in core/includes/common.inc.

Filed CR https://www.drupal.org/node/3014611

Fixed summary and title

PS: the deprecated constant was added in #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list

Committed 0a6e463 and pushed to 8.7.x. Thanks!