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 database path in Thelia Update #1729

Merged
merged 1 commit into from Oct 14, 2015

Conversation

Projects
None yet
4 participants
@Alban-io
Contributor

Alban-io commented Oct 13, 2015

THELIA_CONF_DIR constant already contains DIRECTORY_SEPARATOR.

@gillesbourgeat

This comment has been minimized.

Show comment
Hide comment
@gillesbourgeat
Member

gillesbourgeat commented Oct 13, 2015

👍

gillesbourgeat added a commit that referenced this pull request Oct 14, 2015

@gillesbourgeat gillesbourgeat merged commit 4d5c18c into thelia:master Oct 14, 2015

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bibich bibich referenced this pull request Oct 16, 2015

Merged

Version 2.2.1 #1736

bibich added a commit to bibich/thelia that referenced this pull request Oct 19, 2015

@bibich bibich referenced this pull request Oct 19, 2015

Merged

Version 2.1.7 #1737

@judgej

This comment has been minimized.

Show comment
Hide comment
@judgej

judgej Oct 30, 2015

Do you need most of the directory separators that are left? PHP has standardised on "/" for all paths it uses, and will convert to whatever the OS needs when the path is used. The exception is when passing paths into an OS shell command, but that is something seldom done.

judgej commented on 23ac656 Oct 30, 2015

Do you need most of the directory separators that are left? PHP has standardised on "/" for all paths it uses, and will convert to whatever the OS needs when the path is used. The exception is when passing paths into an OS shell command, but that is something seldom done.

This comment has been minimized.

Show comment
Hide comment
@roadster31

roadster31 Oct 31, 2015

Contributor

Well, no. __DIR__ is something like E:\Dev\Thelia\Thelia2\thelia\web on Windows, with PHP 5.4.

Contributor

roadster31 replied Oct 31, 2015

Well, no. __DIR__ is something like E:\Dev\Thelia\Thelia2\thelia\web on Windows, with PHP 5.4.

This comment has been minimized.

Show comment
Hide comment
@judgej

judgej Oct 31, 2015

You can still use __DIR__ . '/whatever/directory/you/like' to open a file without having to know anything about what directory separators the OS uses.

judgej replied Oct 31, 2015

You can still use __DIR__ . '/whatever/directory/you/like' to open a file without having to know anything about what directory separators the OS uses.

This comment has been minimized.

Show comment
Hide comment
@roadster31

roadster31 Nov 1, 2015

Contributor

Sure. But any path string manipulation relying on '/' will fail on Windows. That's why using something like DS is fine.

Contributor

roadster31 replied Nov 1, 2015

Sure. But any path string manipulation relying on '/' will fail on Windows. That's why using something like DS is fine.

This comment has been minimized.

Show comment
Hide comment
@judgej

judgej Nov 1, 2015

I'm not here to argue about edge-cases. There is a code smell, and I was pointing it out. Statements like require __DIR__ . DIRECTORY_SEPARATOR . $arg; seem to fit under the category of "useless DIRECTORY_SEPARATOR" to me.

But that's me done. Take the point or leave it.

judgej replied Nov 1, 2015

I'm not here to argue about edge-cases. There is a code smell, and I was pointing it out. Statements like require __DIR__ . DIRECTORY_SEPARATOR . $arg; seem to fit under the category of "useless DIRECTORY_SEPARATOR" to me.

But that's me done. Take the point or leave it.

@gillesbourgeat gillesbourgeat added this to the 2.3.0-alpha1 milestone Nov 19, 2015

@Alban-io Alban-io deleted the Alban-io:fix-update-database-config-path branch Dec 14, 2015

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