InstallerTranslationExistingFileTest fails on 11.x branch

Created on 18 May 2024, 6 months ago
Updated 3 June 2024, 6 months ago

Problem/Motivation

Failing tests such as https://git.drupalcode.org/project/drupal/-/jobs/1622602

I looked into this a bit. It started with 🐛 Composer tests fail because of invalid Drupal version Fixed Where the version was changed from 11.0.0-dev to 11.0-dev

With the drupal version change in that issue, the test downloads the translation file as drupal-11.0-dev.xx-lolspeak.po instead of drupal-11.0.x.xx-lolspeak.po

I don’t know what’s supposed to happen here with this version, but this line https://git.drupalcode.org/project/drupal/-/blob/11.x/core/includes/inst... would normally change 11.0.0-dev to 11.0.x but it can’t match 11.0-dev, so it’s just leaving it as 11.0-dev. the translation server seems to have files for both 11.x and 11.0-dev so regardless, it gets a file.

But the file translator here: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... won’t match either of those two translation file patterns. So the installer sees the language request, sees it doesn’t have a translation file, orders it downloaded, the download succeds, installer_download_translation reloads the page (purportedly in the new language), and on reload, the installer sees a language request, but doesn’t see the file that had been downloaded, and starts the process over again until something times out.

Good news is this should only affect the 11.x branch, and not the alpha or beta releases.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Fixed

Version

11.0 🔥

Component
Install 

Last updated 1 day ago

No maintainer
Created by

🇺🇸United States mikelutz Michigan, USA

Live updates comments and jobs are added and updated live.
  • Needs framework manager review

    It is used to alert the framework manager core committer(s) that an issue significantly impacts (or has the potential to impact) multiple subsystems or represents a significant change or addition in architecture or public APIs, and their signoff is needed (see the governance policy draft for more information). If an issue significantly impacts only one subsystem, use Needs subsystem maintainer review instead, and make sure the issue component is set to the correct subsystem.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mikelutz
  • 🇺🇸United States mikelutz Michigan, USA

    I’m not sure where it should be fixed, (more specifically I’m not sure what translation file name format we WANT to use for 11.0-dev, but once we know, we need to adjust either of those two regexes above to download and/or find the right file)

  • Pipeline finished with Success
    6 months ago
    Total: 603s
    #176268
  • 🇺🇸United States mikelutz Michigan, USA

    Here's the simplest fix I see for this test, making the patch value optional in the regex that filetranslation uses to detect files.

    Also, I was trying to figure out why the version change was committed 9 days ago, and we are just seeing this test fail now. The drupal-11.0-dev.xx-lolspeak.po file that is downloaded has metadata indicating it's the file for 11.0.0-beta1. I suspect that file was added to the server with the beta release late last week. The test only tests that a 200 status is returned after selecting a language. If the download fails, the installer displays a requirements page stating that the file is missing rather than trying to reload the page, so the test passes if the download fails. So it wouldn't have been a problem until the download succeeded, but the installer couldn't detect the downloaded file.

  • Status changed to Needs review 6 months ago
  • 🇺🇸United States mikelutz Michigan, USA
  • 🇺🇸United States mikelutz Michigan, USA

    I'm wondering if we should do something more robust here, like add a query parameter on the redirect indicating that we downloaded the translation file, so when we go back through we see that we thought we downloaded it, but can't find it and report the error instead of an endless redirect. Or a more structured way of ensuring that whatever is set as the drupal version will pass the FileTranslation scanner. I'm really not sure, It's been a while since I got deep in the installer code, and I'm not that familiar with the deep parts of the string translation system and po files to know what else could be screwed up by changing the FileTranslation scanner. I suspect the change here is probably safe, but still don't know if it's the right thing to do.

  • 🇺🇸United States mikelutz Michigan, USA

    Moving to critical per https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

    Cause tests to fail in HEAD on the automated testing platform for any supported environment (including random failures), since this blocks all other work.

  • Status changed to RTBC 6 months ago
  • 🇬🇧United Kingdom catch

    I think we should open a follow-up for #6 to make things more robust, but also let's go ahead here to un-break HEAD again. All of this will change again once we can use a main branch instead of 11.x.

    • larowlan committed b1c945e4 on 11.x
      Issue #3448036 by mikelutz: InstallerTranslationExistingFileTest fails...
  • Status changed to Fixed 6 months ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Thanks, committed to 11.x

  • 🇳🇿New Zealand quietone

    Tagging per #8

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

Production build 0.71.5 2024