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

Fix a path generation issue in Propel class generators #2511

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@bcbrr
Contributor

bcbrr commented Jul 5, 2018

The endPath arguments passed to Symfony\Component\Filesystem\Filesystem::makePathRelative() had a double path separator.
For the path that has a .. this caused an issue with path generation: the .. segment was not properly applied because of this double separator.

Fixes #2490

Fix a path generation issue in Propel class generators
The `endPath` arguments passed to `Symfony\Component\Filesystem\Filesystem::makePathRelative()` had a double path separator.
For the path that has a `..` this caused an issue with path generation: the `..` segment was not properly applied because of this double separator.

Fixes #2490
@roadster31

This comment has been minimized.

Contributor

roadster31 commented Jul 5, 2018

👍

@roadster31

This comment has been minimized.

Contributor

roadster31 commented Jul 5, 2018

I'm working with windows (OK, I know), and I can see that there's a mix with \ and / in path, meaning that sometimes "/" is used to build path instead of the DS constant.

@bcbrr

This comment has been minimized.

Contributor

bcbrr commented Jul 5, 2018

I changed these two classes to use DS.

However, makePathRelative is converting Windows path separators to UNIX path separators, but does not convert back when returning. Should this be done ? (There is four uses of this method in Thelia : these 3, and one in ModuleGenerateCommand).

I also used / separators in #1975, should I rework it to change everything to DS ?

@roadster31

This comment has been minimized.

Contributor

roadster31 commented Jul 5, 2018

However, makePathRelative is converting Windows path separators to UNIX path separators, but does not convert back when returning. Should this be done ?

I suppose, as mixin / and \ in path will cause problems.

I also used / separators in #1975, should I rework it to change everything to DS ?

I did not review this PR. But IMO, if you're working on file system paths, you should be consistent and use DS everywhere.

For example, something like rtrim(aPath, '/') will not work on Windows, but is not noticeable on Unix-like systems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment