Skip to content
This repository was archived by the owner on Jul 16, 2020. It is now read-only.

Use DIRECTORY_SEPARATOR instead of hardcoded chars #83

Merged
merged 2 commits into from
May 23, 2019

Conversation

wdacgrs
Copy link
Contributor

@wdacgrs wdacgrs commented May 14, 2019

Replaced the '/' and '\' to DIRECTORY_SEPARATOR.

Replaced the '/' and '\\' to DIRECTORY_SEPARATOR.
$root = realpath(base_path() . $this->config['langPath'] . '/' . $lastLocale);
$filePath = $this->removeExtension(str_replace('\\', '_', ltrim(str_replace($root, '', realpath($fileName)), '\\')));
$root = realpath(base_path() . $this->config['langPath'] . DIRECTORY_SEPARATOR . $lastLocale);
$filePath = $this->removeExtension(str_replace(DIRECTORY_SEPARATOR, '_', ltrim(str_replace($root, '', realpath($fileName)), DIRECTORY_SEPARATOR)));
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure about this change. Assuming / is not the same as , the code no longer does the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code only works in Linux and Mac environment. For my use case, the generator needs to be executed under Windows.

When executed under Windows environment, the $filePath does not contain a leading '/' and hence the first character of the file name is mistakenly removed which makes the first level key (the one under locale) in the generated file wrong.
For example:

'en': {
    'filename': {

becomes

'en': {
    'ilename': {

I think we will need some more changes to make it works the original way.
I just tried the modified code under Windows and Mac for comparison and found that for subdirectory 'a' and a 'b.php' inside it, Windows created a file called 'a_b.js' while Mac created a subdirectory 'a' and then put a 'b.js' inside it.

Copy link
Owner

Choose a reason for hiding this comment

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

A few more tests should go with these changes so we don't break existing behavior on Mac/Linux.

Thanks for doing this!

if ($multiLocales) {
$this->filesToCreate[$lastLocale][$lastLocale][substr($filePath, 1)] = $this->adjustArray($tmp);
$this->filesToCreate[$lastLocale][$lastLocale][$filePath] = $this->adjustArray($tmp);
Copy link
Owner

Choose a reason for hiding this comment

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

Also not sure about this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer need to remove the leading '/' since it should be already removed at this point.

Conditionally remove the leading '/' in $filePath instead of replacing DIRECTORY_SEPARATOR.
@martinlindhe martinlindhe merged commit 841d82b into martinlindhe:master May 23, 2019
@martinlindhe
Copy link
Owner

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants