Problem/Motivation

While attempting to run the update-countries.sh script with an appropriate territories.json file, the resulting CountryManager.php file consisted of country codes mapped to t() verbatim (no country name within quotes) unlike the original CountryManager.php file provided with composer, which is correctly formatted.

This results in the country names being removed from the file and subsequent runs of update-countries.sh file to error on an invalid call to t() in CountryManager.php.

Steps to reproduce

Place the requested territories.json file in DRUPAL_ROOT and run core/scripts/update-countries.sh. The resulting core/lib/Drupal/Core/Locale/CountryManager.php will be "corrupted" with lines such as

'AC' => t(),

instead of

'AC' => t('Ascension Island'),

This was tested with PHP 8.3 and Drupal 10.3.5. The file appears to have the same bug in v11.x too.

Proposed resolution

A. Line 96 of core/scripts/update-countries.php needs to be changed from

$name = str_contains($name, '\'' ? '"' . $name . '"' : "'" . $name . "'");

to

$name = str_contains($name, '\'') ? '"' . $name . '"' : "'" . $name . "'";

Note the misplaced closing bracket inadvertently wrapping the entire ternary rather than just the two parameters to the str_contains function.

B. Restore the fake t() function.

Remaining tasks

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

Issue fork drupal-3478408

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

goonerw created an issue. See original summary.

quietone’s picture

Version: 10.3.x-dev » 11.x-dev
Issue tags: +Needs tests

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Also, I reproduce this problem.

binoli lalani made their first commit to this issue’s fork.

jaydev bhatt made their first commit to this issue’s fork.

jaydev bhatt’s picture

Status: Active » Needs review

I have tested the updated code on Drupal 11.1.1 and reviewed the changes thoroughly. With the updates made to the script, the update-countries.sh file now runs without any issues. Additionally, I have ensured that the code aligns with the latest version of the 11.x branch.

I am moving this issue to Needs Review (NR) for further verification. However, based on my testing and review, this patch appears to be ready for RTBC (Reviewed & Tested by the Community).

Thank you!

smustgrave’s picture

Status: Needs review » Needs work

Was previously tagged for tests which are still needed it appears.

jaydev bhatt’s picture

Assigned: Unassigned » jaydev bhatt

I am working on writing PHPUnit test cases for the update-countries script. The test currently verifies the following:
• The script correctly reads and processes the territories.json file.
• Excluded country codes (e.g., ‘EU’, ‘UN’, ‘ZZ’) are not included in the final country list.
• The updated country list is properly written to CountryManager.php.

Are there any additional test cases that should be considered to ensure full coverage? Feedback is welcome!

jaydev bhatt’s picture

Assigned: jaydev bhatt » Unassigned

Upon debugging the issue further and for writing the test properly.
found out that the issue which is mentioned in the description to run the update-countries.sh to Update the CountryManager.php

here the .sh file it self is generating the error with the 11.x code base and not updating the CountryManager.php file.

i setup the drupalsetup using ddev and was trying to run the sh file like this ddev exec php core/scripts/update-countries.sh
and got the following error

PHP Fatal error:  Uncaught Error: Call to undefined function Drupal\Core\Locale\t() in /Users/abc/Developer/DrupalProject/core/lib/Drupal/Core/Locale/CountryManager.php:45
Stack trace:
#0 /Users/abc/Developer/DrupalProject/core/scripts/update-countries.sh(43): Drupal\Core\Locale\CountryManager::getStandardList()
#1 {main}
  thrown in /Users/abc/Developer/DrupalProject/core/lib/Drupal/Core/Locale/CountryManager.php on line 45

keep this on need work as need more input on this error of the .sh file.

poker10’s picture

I think that this probably does not need tests, as it is a simple typo from a previous issue (see this commit). Checking by https://www.drupal.org/about/core/policies/core-change-policies/core-gat... :

1. The issue has clear ‘Steps to reproduce’ in the Issue Summary. - YES
2. The fix is 'trivial' with small, easy to understand changes. - YES

Questions
1. Is the fix is easy to verify by manual testing? - YES
2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc. - YES
3. Is the fix achieved without adding new, untested, code paths? - YES
6. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed? - YES

What do you think?

catch’s picture

Agreed with #10. Also if we'd noticed this quickly we might have reverted the original issue and we don't add new year coverage with a revert.

quietone’s picture

Title: update-countries.sh inadvertently broken by Issue #3328454 » Fix errors in update-countries.sh
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

OK, so no tests.

However, with that 'typo' fixed the script fails to run.Doing so results in the error reported by @jaydev bhatt, in #9. The solution in the MR is too bootstrap Drupal so that t() is found. That seems unnecessary since this isn't doing any translations. So, I did some digging.

$ git log -- core/scripts/update-countries.sh reported #3384436: Remove redefintion of t() from update-countries.sh was committed in Sep 2023. Reading that issue brought back memories of being here before. Now that there is an argument in the t() functions in CountryManager I think the fake t() removed in #3384436 can be restored.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Consensus seems no test then change seems straight forward

  • longwave committed c6ae57bf on 10.4.x
    Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in update-...

  • longwave committed 7b58a06d on 10.5.x
    Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in update-...

  • longwave committed 6c1360bc on 11.1.x
    Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in update-...

  • longwave committed 61dfd43e on 11.x
    Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in update-...

longwave’s picture

Backported down to 10.4.x just in case someone actually wants to run this script on an older version, although it shows how often we use this if it's been broken for 18 months.

Committed and pushed 61dfd43e036 to 11.x and 7b58a06d834 to 10.5.x and 6c1360bc84b to 11.1.x and c6ae57bfb72 to 10.4.x. Thanks!

longwave’s picture

Status: Reviewed & tested by the community » Fixed

  • catch committed b7f8d6d9 on 10.4.x
    Revert "Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in...

  • catch committed 98ca5625 on 10.5.x
    Revert "Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in...
catch’s picture

Status: Fixed » Needs work

this broke phpstan. Reverted from all four branches. https://git.drupalcode.org/project/drupal/-/jobs/5139702

  • catch committed 59ecaafa on 11.1.x
    Revert "Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in...

  • catch committed 85836e3c on 11.x
    Revert "Issue #3478408 by jaydev bhatt, quietone, goonerw: Fix errors in...
bbrala’s picture

Maybe we can exclude this file from phpstan?

godotislate made their first commit to this issue’s fork.

godotislate’s picture

Status: Needs work » Needs review

MR https://git.drupalcode.org/project/drupal/-/merge_requests/12029 restoring the fix and adding an excludePaths entry for update-countries.sh to phpstan. Tests pass, but not completely sure that proves anything since the original MR PHPStan job passed too.

bbrala’s picture

Status: Needs review » Reviewed & tested by the community

Merged these changes into the branch I detected the failure in earlier today. Checked if i still got the failure as i did then. I didnt, so I am pretty sure we are all good now. ;)

longwave’s picture

Status: Reviewed & tested by the community » Fixed

Second time lucky? Changed my mind about backporting, not worth going to 11.2.x/10.5.x here now.

Committed and pushed to 11.x and 10.6.x.

  • longwave committed 93e785c1 on 10.6.x
    Issue #3478408 by jaydev bhatt, quietone, godotislate, goonerw, catch:...

  • longwave committed 38bcbe8e on 11.x
    Issue #3478408 by jaydev bhatt, quietone, godotislate, goonerw, catch:...

longwave’s picture

Status: Fixed » Closed (fixed)

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