-
Notifications
You must be signed in to change notification settings - Fork 214
Use DIRECTORY_SEPARATOR instead of hardcoded chars #83
Conversation
Replaced the '/' and '\\' to DIRECTORY_SEPARATOR.
src/Generator.php
Outdated
$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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Thanks! |
Replaced the '/' and '\' to DIRECTORY_SEPARATOR.