Problem/Motivation

The order of the projects from locale_translation_get_projects() is not consistent. If the project list is rebuilt it will be in the order returned from locale_translation_project_list(). This might be altered to ensure a particular translation project is last. However, once the cache has been built the order of the projects is in whatever order \Drupal::service('locale.project')->getAll() returns them from the database.

The order needs to be consistent and never change if the underlying code does not change i.e. module weights / alters etc.

Steps to reproduce

Proposed resolution

Order by key alphabetically, as this is the current behaviour once the projects have been stored in the storage, but allow alters to set a wright key that can change the order to ensure translations come before or after each other as desired.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

New optional weight key in project information.

Release notes snippet

CommentFileSizeAuthor
#12 3403337-nr-bot.txt1.74 KBneeds-review-queue-bot

Issue fork drupal-3403337

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:

About issue forks

Comments

alexpott created an issue. See original summary.

alexpott opened merge request !5507

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Issue tags: +Needs change record
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -Needs change record

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

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

quietone’s picture

quietone commented

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.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3403337-nr-bot.txt1.74 KB

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.

akhil babu’s picture

akhil babu
he/him
commented
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Feedback has been addressed

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

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.

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

vakulrai’s picture

vakulrai commented
Status: Needs work » Needs review

Updated the comments in the MR , please review !

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@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.

quietone’s picture

quietone commented

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

quietone’s picture

quietone commented

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Reviewed and appears that feedback has been addressed

  • catch committed cc3f4029 on 10.3.x 
    Issue #3403337 by alexpott, quietone, Akhil Babu, vakulrai, smustgrave,...

  • catch committed 36fece71 on 11.x 
    Issue #3403337 by alexpott, quietone, Akhil Babu, vakulrai, smustgrave,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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!

  • longwave committed f0be4c1c on 10.3.x 
    Revert "Issue #3403337 by alexpott, quietone, Akhil Babu, vakulrai,...

  • longwave committed d7602652 on 11.x 
    Revert "Issue #3403337 by alexpott, quietone, Akhil Babu, vakulrai,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Fixed » Needs work

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.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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.

  • longwave committed c1634b16 on 10.3.x 
    Issue #3403337 by alexpott, quietone, Akhil Babu, vakulrai, smustgrave,...

  • longwave committed e7e4e0e7 on 11.x 
    Issue #3403337 by alexpott, quietone, Akhil Babu, vakulrai, smustgrave,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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