fix: normalize filename and try to find a match#33
Conversation
|
I'm not sure why Code Climate thinks the total coverage dropped. When I go to them for details, the page is empty. |
| ); | ||
| } | ||
| $options->weekday = ['short', 'long', 'narrow', 'short'][$length - 4]; | ||
| $options->weekday = $this->getWeekdayValue($length - 4); |
There was a problem hiding this comment.
This fixes a static analysis issue that Psalm raised in CI.
There was a problem hiding this comment.
Okay because I don't understand what a narrow weekday is :)
There was a problem hiding this comment.
These values are the ECMA-402 names that translate to ICU formatting symbols. "Narrow" basically means the shortest possible display. So, while long is "Friday" and short is "Fri," narrow is "F"
| return $this->formatHelper->getReader($formatReader); | ||
| } | ||
|
|
||
| private function getFilePathForLocale(string $locale): string |
There was a problem hiding this comment.
This method fixes the case-sensitivity problem.
| { | ||
| $normalize = fn (string $filename): string => str_replace('_', '-', strtolower($filename)); | ||
| $searchFile = $normalize($locale . self::MESSAGE_FILE_EXTENSION); | ||
| $localeFiles = scandir($this->messagesDirectory, SCANDIR_SORT_NONE) ?: []; |
There was a problem hiding this comment.
Would it be worth putting in a short-circuit before the scandir in case the file can be found directly?
Just thinking of cases where messagesDirectory might have a bunch of files in it and everything is named (and cased) properly, so we can save the disk IO.
|
|
||
| ### Fixed | ||
|
|
||
| - Normalize the locale file name before searching for it in `MessageLoader`, to account for differences in case, as well as filesystem case sensitivity (e.g. "en-XB" vs. "en_xb") |
There was a problem hiding this comment.
Did you mean (e.g. "en-XB" vs. "en-xb") (both hiphens)?
There was a problem hiding this comment.
I meant en_xb. Even if someone names their locale files EN_XB.json, EN-XB.json, EN-xb.json, etc., the locale en-XB should match.
|
|
||
| private function getFilePathForLocale(string $locale): string | ||
| { | ||
| $normalize = fn (string $filename): string => str_replace('_', '-', strtolower($filename)); |
jrode
left a comment
jrode
left a comment
There was a problem hiding this comment.
Looks great, nice catch. One non-blocker.
|
Code Climate has analyzed commit c38f784 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 90.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 96.8% (-0.1% change). View more on Code Climate. |
3 participants
We've been scratching our heads over an issue that appeared to work locally but never in sandbox, staging, or production environments.
Then, I had an ah-ha moment and tried this. First, on my Mac:
Then in a sandbox environment:
Yes, I know the macOS filesystem is case-insensitive, but even when using Linux containers, the directory mounted to the container IS STILL CASE-INSENSITIVE.
Our server environments are all Linux and, therefore, case-sensitive. 💥
The logic we are using to create the file path for looking up messages is, in short:
Since we're normalizing the locale name (i.e.,
en-xbanden_XBbecomesen-XB), in a local environment, it works out great becauseen-XB.jsonanden-xb.jsonare the same files. However, in a server environment, since our locale files are lowercased (i.e.,en-xb.json), it can't find these files, when running on the servers.This PR normalizes the file names and searches through the files to find the correct one, comparing the normalized names, rather than concatenating the strings and hoping the files exist.
Fixes SK-36605
Product requirements and context
How has this been tested?
Tested in sandbox-3 by loading pages with different locales and confirmed this fixes the problem.
PR Checklist