The order of the projects coming from locale_translation_get_projects() is not consistent

I think this makes sense. Why do you need to add the NULL filter though? How is that related?

@Gábor Hojtsy because LocaleProjectStorage is has several bugs - if you don't add the NULL filter than the key will remain in the ouput if you do ::getAll after a ::delete... the new test added here confirms tests this behaviour and leaving the bug in felt irresponsible.

Also fixed here is changing the property all from a static to a regular class property because that has no business being static when $cache is not.

The one thought here is should we obey the order generated by locale_translation_project_list() instead or adding a weight key and sorting by key.

The sorting by key behaviour is the current behaviour - unless you've just called locale_translation_build_projects() for the first time. At that point it will be in the order that locale_translation_project_list() returns until the process ends. Any subsequent process will get it in the key order.

I went for the current change because I think it is less risky for current sites which might be relying on the current order to have translations work out the way they want.

Code-wise LGTM.

The only issue we can have with this is altering what people is expecting on their projects. But given we are actually making it deterministic, any expectation they had before this MR is merged is actually wrong and not reliable. So it's definitely not making things worse for them, actually providing a way to fix it via weight if they need to.

So IMHO this is RTBC.

Created the CR - https://www.drupal.org/node/3404117 →

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.

I read the MR and found a grammar error which I fixed, and I also rebased. The change record also had a sentence that needed rewording so I did that too. I also moved the example to a separate heading.

I tested locally and the test does fail without the code changes.

I also updated credit.

Since the change to the MR was minor in a comment I am leaving at RTBC.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Akhil Babu → made their first commit to this issue’s fork.

Akhil Babu → changed the visibility of the branch 11.x to hidden.

Added declare(strict_types=1) to the test as per #12. Moving to needs review

Left some small updates. Leaving in NR for additional eyes.

Feedback has been addressed

I'm triaging RTBC issues → . I read the IS, the comments and the MR.

I left some work in the MR, so setting to needs work. I just noticed that I suggested a change that alexpott removed but I have to go right now and can't follow up.

Updated the comments in the MR , please review !

@vakulrai
return array_filter($this->cache, fn ($value) => $value !== NULL);
is not the same as
return array_filter($this->cache);
The latter will remove empty arrays, FALSEs and empty strings which is not the current behaviour. We only want to remove the NULLs and the \Drupal\Tests\locale\Kernel\LocaleTranslationProjectsTest::testLocaleTranslationClearCacheProjects() relied on this.

So, I make comments on the MR but didn't save them. I have done that now but now the conversation isn't in the correct order.

The result of the last day here is changes to comments. And I just made another one to correct tense. The questions I raised in the MR resulted in code changes that were reverted by alexpott, with explanation.

Reviewed and appears that feedback has been addressed

Reviewing this I had a similar thought process to #7 and would have leaned on respecting the order of local_translation_project_list() but I think that would have taken even more refactoring, so adding the explicit weight key seems OK.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Reverted, as the new unit test fails on PHP 8.3 (but not 8.1 or 8.2):

$ php8.3 vendor/bin/phpunit -c core core/modules/locale/tests/src/Unit/LocaleProjectStorageTest.php
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\locale\Unit\LocaleProjectStorageTest
F.                                                                  2 / 2 (100%)

Time: 00:00.471, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\locale\Unit\LocaleProjectStorageTest::testSorting
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => 'd'
-    1 => 'a'
-    2 => 'b'
-    3 => 'c'
+    0 => 'a'
+    1 => 'b'
+    2 => 'c'
+    3 => 'd'
 )

/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/var/www/html/drupal/core/modules/locale/tests/src/Unit/LocaleProjectStorageTest.php:58
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 2, Assertions: 9, Failures: 1.

Turns out this is a PHP 8.3 bug - see https://github.com/php/php-src/pull/13285 - fortunately a work around is pretty simple - see https://git.drupalcode.org/project/drupal/-/merge_requests/5507/diffs?co...

Also in the interim testing is now defaulting to PHP 8.3 so if we the main pipeline is green we know we're fixed it.

Nice that we found a PHP bug, even better that it's already fixed - and that we can work around it here.

Committed and pushed e7e4e0e776 to 11.x and c1634b16eb to 10.3.x. Thanks!

longwave → closed merge request !5507

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