Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #6717: Fix faulty version pattern in translation download #4873

Open
wants to merge 2 commits into
base: 1.x
Choose a base branch
from

Conversation

indigoxela
Copy link
Member

@backdrop-ci
Copy link
Collaborator

Related to: backdrop/backdrop-issues#6717

@dragonbot
Copy link
Collaborator

Tugboat has finished building a preview for this pull request!

Website: https://pr4873-nvqbbkvloukpxu45moxpbbombj1cvjiu.tugboatqa.com/
Username: admin
Password: d48cbbc3fa27

This preview will automatically expire on the 18th of November, 2024.

// Look up the version specific translation file. We can not rely on its
// existence, as generation happens manually after release.
// If it does not exist, yet, we stick with the generic fallback.
if (!install_check_localization_server($translation_url)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately another request to the server's unavoidable (unless I'm missing something). But it's a lightweight HEAD request, anyway.

Copy link
Contributor

@hosef hosef Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better if we change install_check_localization_server() so it will continue to check if it gets a request in the 400 range? That implies that the translation site is fine, but the uri we used is not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or at least add a flag so we can stop it from setting $online = FALSE in this one use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function install_check_localization_server() is actually quite smart. It avoids any additional checks, if a previous one already failed. If the request to the server (without path) doesn't work at all, don't waste time with subsequent requests to the same server (timeout is only 5 seconds, but that would sum up). The most common case for failure will be that there's either no internet connection at all (local offline install), or some PHP problem (not able to do external requests).

Whether it's a 404 actually gets relevant in our last check - for the version specific translation file.
I couldn't think of a better logic for these checks or their order. @hosef or do you have something smarter in mind?

Remember, it's just a single (cheap) additional HEAD request, anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure. I took a look at the function and it just feels like a simple 200 = good, all other codes = bad is way too simple for what could actually happen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is probably fine as is because we control the site being requested here, so we control the response codes. I've been working on writing some API clients recently, and it just feels bad that we are not properly handling the other response codes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit, http handling's quite simple here: 200=good anything_else=bad. But I think, in this case this approach is fine.

We're not interested whether anything moved (3xx) or whether something's temporarily broken (5xx) or the file just isn't there (404). We quickly inform about problems if necessary, hide those we can silently fix (fallback file), and suggest an immediate solution via UI if not. To not make the install process cumbersome because of problems outside this (not yet) install.

All of the "not 200" mean, that we can't get the translation file right now. So why bother?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why bother?

So we don't have to check the fallback uri before checking the uri we actually want.

Unfortunately another request to the server's unavoidable (unless I'm missing something).

The thing you are missing is proper response code handling. The server tells us why it didn't work and we are just ignoring it.

It works the way it is. We would just need to add more checks to that function to avoid making 2 requests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing you are missing is proper response code handling.

Am I? 😜

What I could imagine, is a second param for install_check_localization_server() like $ignore_cache (default false) - that would allow to check the fallback after the regular translation file - one request less. If we got that far (we're online, the server's reachable), most likely the "regular" file should be accessible, too (unless the file hasn't been generated, yet).
But that additional param feels a bit odd and makes that utility function logic harder to understand. I might be wrong, though.
The actual reason, why the translation file isn't there isn't actually relevant IMO. We can't have it - try the fallback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hosef are you still interested in this PR? Any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translation not available during installation
4 participants