update-countries.sh inadvertently broken by Issue #3328454

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.

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!

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

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!

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.

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?

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.

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 🐛 Remove redefintion of t() from update-countries.sh Fixed 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.

Consensus seems no test then change seems straight forward

longwave → closed merge request !10324

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!

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

Maybe we can exclude this file from phpstan?

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.

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. ;)