Problem/Motivation
If an existing module gets replaced by a new version of this module, the new version is not displayed on the translation status page.
Proposed resolution
Refresh the data during the module update(.php) process.- Refresh the data and remove old translation file during translation update (cron, drush, manual)
Remaining tasks
t.b.d.
User interface changes
bug is fixed
API changes
none
Data model changes
none
Comment | File | Size | Author |
---|
Issue fork drupal-2575945
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:
- 2575945-a-new-module changes, plain diff MR !5616
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedA simple fix. This clears the 'last updated date' too, perhaps that is not a good idea.
Comment #3
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedComment #9
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedI have found a new approach, more subtle one. It clears the translation status and removes the translation file(s) when a change in version is detected.
Comment #11
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #13
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRe-rolled the #9 patch against 8.8.x before working on it.
Comment #16
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedTest added that checks the new functionality: checks if project version changes; deletes the old translation file; downloads the new translation.
Existing test fixed and documented.
Comment #20
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRerolled against latest 9.3.x.
Comment #21
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedRemoved the unused type hint that caused a spelling check error.
Comment #23
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedA few compatibility changes in test scripts.
Comment #24
andypostI bet it needs type-hints for the function arguments
Moreover I'd better created separate class for new function in context on "Kill includes" tag but I sure there's follow-up already for that
I'm sure
$this->container
should be replaced with\Drupal::moduleHandler()
Comment #25
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedBoth comments in #24 addressed in this patch.
I did not find an issue for converting any of the functions in this batch.inc. I did find some individual issue to convert the procedural functions to a decent API. I think it is time for a new initiative to move locale module up the OO ladder. It's been procedural for too long.
Comment #26
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #27
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedFor the "Kill includes" I started with this summary: #3097045-14: [META] Provide modern replacements for and deprecate the legacy include files
Comment #28
andypostThank you a lot for #27!
The last
$project, $langcode
both needsstring
type-hint as they available since PHP 7.0Comment #29
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThanks for the quick review.
This patch addresses the #28 comments.
Comment #30
andypostThanks, looks ready
Comment #31
tobiasbRemoved typehint of $context in
locale_translation_batch_version_check
.phpDoc allows
\ArrayAccess
and drush use this. ;-)Comment #33
tobiasbComment #34
tobiasbFixed deprecation.
assertFileNotExists -> assertFileDoesNotExist.
Comment #35
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedPatch is working fine and applied cleanly ....
Thanks
Comment #38
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRerolled #34 for 9.5
Comment #41
ericdsd CreditAttribution: ericdsd commentedHi i have the same issue with drush locale:update that doesn't replace contrib module .po file by new version when module is updated even after a locale:update locale:check.
Experiencing it on drupal 9.4.8
Comment #42
ericdsd CreditAttribution: ericdsd commentedHi patch #38 works against 9.5.2
tested with both config remote_and_local and Local
it works nicely, the only strange thing is that the conts of updated strings slihtly differs on each test i did event while using the same database as start point.
When rexproting the sites po for comparison, everything looks fine, i only have 2 extra string when i'm in remote_and_local mode as those strings are displayed during remote locale:check.
with same database :
remote+local
> [notice] Translations imported: 847 added, 13712 updated, 0 removed.
local (with po commited from first test)
> [notice] Translations imported: 848 added, 13468 updated, 0 removed.
Comment #43
mstrelan CreditAttribution: mstrelan at PreviousNext commentedCame here from a related issue in Bug Smash. Have updated the patch in #38 to fix the test fail / deprecation. Also since we can have union types now I've reverted #31 and added a union type.
Comment #44
smustgrave CreditAttribution: smustgrave at Mobomo commentedLocaleTranslationChangeProjectVersionTest - Error : Call to undefined function Drupal\Tests\locale\Functional\locale_translation_batch_version_check()
testUpdateCron -
Queue holds tasks for one project.
Failed asserting that 2 matches expected 3.
Expected :3
Actual :2
Made sure the tests fail without the fix and they do.
Reviewing the code changes look good.
Question for committer if locale_translation_batch_version_check needs a change record.
Comment #47
Ammaletu CreditAttribution: Ammaletu at Zebralog commentedI would be interested in getting this finished and merged. This might not be the worst bug that Drupal has to offer, but it still breaks a pretty basic core functionality. Right now, people are upgrading to Drupal 10.1 and will be wondering why all kinds of simple and visible Strings are not translated.
I just added the patch #43 to our Drupal 10.1.6 installation. It applied without a problem and cured the problem with the missing translations of newer Strings. The new test LocaleTranslationChangeProjectVersionTest.php runs successfully, as does the changed test LocaleUpdateCronTest.
I'm not sure about Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest that apparently failed back in May, see #45. Our project still uses CKEditor 4, but I tried it on an unmodified Drupal 10.1 installation. Everything worked there, but all test methods in the MediaTest were skipped.
So what is actually mssing here? I tried to trigger a restest of patch #43. When this hopefully succeeds, can we simply set this back RTBC?
Comment #48
Ammaletu CreditAttribution: Ammaletu at Zebralog commentedIn the meantime, I had a second look and understood that I need to install a Chromium driver so that the JavaScript tests are not simply skipped. With ddev-selenium-standalone-chrome installed, they are now executed. In my local test setup of a fresh Drupal 10.2-dev installation, JSWebAssertTest works with the patch installed as does MediaTest.
Can somebody with a better understanding of how Drupal's test setup works please have a look at what might be missing here or if we can set this back to RTBC? I'm setting this back to "Needs review" for now.
Also, comment #44 raised the question "if locale_translation_batch_version_check needs a change record". I'm not sure about that -- do we need a change record just because there is a new function? But I think fixing the bug might warrant a change record, as people might rely on the old translations or have lots of missing translations added themselves.
Comment #49
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #51
mstrelan CreditAttribution: mstrelan at PreviousNext commentedJavaScript tests tend to have random fails. I've converted to a merge request rather than rerunning the failing job since it's a lot quicker. Let's see if it comes back green.
Comment #52
mstrelan CreditAttribution: mstrelan at PreviousNext commentedComment #53
smustgrave CreditAttribution: smustgrave at Mobomo commentedMR is showing green, believe #43 was a random failure.
Thanks @mstrelan for hiding all patches.
Comment #56
larowlanCommitted to 11.x
Backported to 10.2.x
Nice work folks, great to get this long-standing issue solved 🐛🔨