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
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
Comment #2
quietone commentedChanges 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.
Comment #6
jaydev bhatt commentedI 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.shfile 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!
Comment #7
smustgrave commentedWas previously tagged for tests which are still needed it appears.
Comment #8
jaydev bhatt commentedI 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!
Comment #9
jaydev bhatt commentedUpon 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.shto Update theCountryManager.phphere 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.shand got the following error
keep this on need work as need more input on this error of the .sh file.
Comment #10
poker10 commentedI 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?
Comment #11
catchAgreed 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.
Comment #12
quietone commentedOK, 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.shreported #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.Comment #13
smustgrave commentedConsensus seems no test then change seems straight forward
Comment #19
longwaveBackported 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!
Comment #20
longwaveComment #23
catchthis broke phpstan. Reverted from all four branches. https://git.drupalcode.org/project/drupal/-/jobs/5139702
Comment #26
bbralaMaybe we can exclude this file from phpstan?
Comment #29
godotislateMR 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.
Comment #30
bbralaMerged 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. ;)
Comment #31
longwaveSecond 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.
Comment #35
longwave