yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.23k stars 6.91k forks source link

Advanced application template and DIRECTORY_SEPARATOR #578

Closed creocoder closed 11 years ago

creocoder commented 11 years ago

Yii 2 in its core use for path construction DIRECTORY_SEPARATOR, always. Since advanced application not use it. For example:

'vendorPath' => dirname(dirname(__DIR__)) . '/vendor',

I suggest analize all places where path is consruct and use DIRECTORY_SEPARATOR for it to be consistent.

qiangxue commented 11 years ago

I don't think we should spend time on this. DIRECTORY_SEPARATOR is actually not always used in the core.

creocoder commented 11 years ago

@qiangxue Then we should replace all DIRECTORY_SEPARATOR to '/' in core. For example what is i want extract vendorPath in chunks using advanced app? I can't. Using framework default vendorPath i can. Lets make all consistent.

creocoder commented 11 years ago

For generating path there should be used DIRECTORY_SEPARATOR or '/', but something one.

qiangxue commented 11 years ago

No, a blind replacement will cause problem. Note that functions like realpath() returns paths with DIRECTORY_SEPARATOR. And sometimes we rely on such fact to do string replacement. In a word, there's no real benefit using '/' everywhere. Let's keep it as is unless you have particular problem.

creocoder commented 11 years ago

@qiangxue Ok, i have problem now with vendorPath extraction in advanced app ;) So why not just make all pathes consistent and generated via DIRECTORY_SEPARATOR? It will be ideologically right and path extraction on every OS can be done via one type of code.

qiangxue commented 11 years ago

If you want to break vendorPath into chunks, then just do a realpath() on it first. Even if the framework does convert everything into DIRECTORY_SEPARATOR, it's not wise to have your code rely on this.

creocoder commented 11 years ago

@qiangxue realpath() make file operations and we (and framework) need avoid this if this possible. This is unacceptable for highload. Why if i can extract path i need do realpath() instead of explode(DIRECTORY_SEPARATOR, $path) ?

qiangxue commented 11 years ago

You don't need to use realpath(). You can use string_replace() to do this, too. But it may end up with "..". Also, by letting the framework to do this normalization, you are shifting the burden to the framework which will degrade the overall performance even if other people don't need to break vendorPath.

creocoder commented 11 years ago

Even if the framework does convert everything into DIRECTORY_SEPARATOR, it's not wise to have your code rely on this.

Yes, i'm argee, but in any case this is at least code style aspect and consistency aspect.

creocoder commented 11 years ago

But it may end up with "..". Also, by letting the framework to do this normalization, you are shifting the burden to the framework which will degrade the overall performance even if other people don't need to break vendorPath.

It cant end with .. if use dirname(dirname(...)) for setting path. Framework should not do normalization ofcourse. Ok. I see it this way: if framework always will use DIRECTORY_SEPARATOR than using some conventions user can without any normalization and sting_replace() get correct path for his OS. Can exec any scripts on Windows from vendors for example without any path pre-transformation. In the end, it's just right use DIRECTORY_SEPARATOR for pathes.

qiangxue commented 11 years ago

This value is set by developer. You can't enforce them to always use DIRECTORY_SEPARATOR. In fact, most people would simply use /, combined with DIRECTORY_SEPARATOR.

creocoder commented 11 years ago

You can't enforce them to always use DIRECTORY_SEPARATOR.

I do not care about how other peoples do it in applications :) This does not matter.

I care about framework should get all pathes correctly because for example in my own application i always use DIRECTORY_SEPARATOR for path and / for urls. If framework will get pathes incorrectly my efforts will be lost :) About advanced app template. It part of framework. It use code style conventions from framework. It should show users how to better do. I do not think we can find any benefit from not using DIRECTORY_DEPARATOR. Moreover we CAN always use it for pathes. At end what problem with do DIRECTORY_SEPARATOR in places where it not used? I can check all framework code and find places where for path DIRECTORY_SEPARATOR not used, correct this and make PR.

samdark commented 11 years ago

What's the benefit using DIRECTORY_SEPARATOR exactly?

creocoder commented 11 years ago

@samdark

In a word, there's no real benefit using '/' everywhere.

It actually means that or DIRECTORY_SEPARATOR or /. Benefits of use is:

As alternative variant we can make:

defined('DS') or define('DS', DIRECTORY_SEPARATOR);

And all can use that. Its simple. Some php frameworks redefine this constant this way.

samdark commented 11 years ago

There are no issues with Windows as far as I know. Even with older versions.

So the only reason is using explode with DIRECTORY_SEPARATOR directly w/o replacing \ to / or doing realpath and that's not reliable anyway because of paths aren't coming from framework only but from OS, third-party components etc.

I see no technical reason doing the change.

creocoder commented 11 years ago

@samdark Ok, lets ask what benefit of not using DIRECTORY_SEPARATOR and using '/' ? If argument is that last is shorter its not argument.

rawtaz commented 11 years ago

@creocoder It is way cleaner and a very common convention for defining a path, and PHP supports / regardless of platform. I don't see why one would not use it, instead of having to concatenate in DIRECTORY_SEPARATOR everywhere.

samdark commented 11 years ago

@creocoder

  1. We're avoiding 2 string concatenations for each usage.
  2. It's shorter w/o any significant drawbacks. Easier to read and understand, no long lines.
  3. Using it in the framework doesn't gurantee that there will be no / on Windows. We don't want to give end users false assumption that it's always safe to do explode(DIRECTORY_SEPARATOR, $path).
creocoder commented 11 years ago

@rawtaz Again. Lets than not use DIRECTORY_SEPARATOR in framework if you say so ;) Its real mess guys ;) How make decision where use DIRECTORY_SEPARATOR or not use? We need think about every place. It take a time. Simple convention about using DS for path generation is make things easier. Or using '/' for path generation always. Simple. But something one.

@samdark

and that's not reliable anyway because of paths aren't coming from framework only but from OS

And? If path come from OS it correct. Its like path coming from framework with DS path generation convention.

third-party components

What third party component? Which not allow code style? Bad component, just not use it.

I see no technical reason doing the change.

What change? Framework in 99% cases use DIRECTORY_SEPARATOR. @qiangxue Just say there is places where its not so and i try to understand why in 1% cases we do not use DS for path generation.

Using it in the framework doesn't gurantee that there will be no / on Windows.

Dont belive. Give example )

klimov-paul commented 11 years ago

At this point I am with @creocoder. It is better to use DIRECTORY_SEPARATOR all over the code, which comes from @yiisoft. Main reason for this is code style consistency. If particular end developer prefer to use “/” instead, it is his concern and his responsibility.

samdark commented 11 years ago

@klimov-paul

$rootDir = __DIR__ . '/../..';

$params = array_merge(
    require($rootDir . '/common/config/params.php'),
    require($rootDir . '/common/config/params-local.php'),
    require(__DIR__ . '/params.php'),
    require(__DIR__ . '/params-local.php')
);

return array(
    'id' => 'app-backend',
    'basePath' => dirname(__DIR__),
    'vendorPath' => dirname(dirname(__DIR__)) . '/vendor',

will become

$rootDir = __DIR__ . '/../..';

$params = array_merge(
    require($rootDir . DIRECTORY_SEPARATOR . 'common' . DIRECTORY_SEPARATOR . 'config' . DIRECTORY_SEPARATOR . 'params.php'),
    require($rootDir . DIRECTORY_SEPARATOR . 'common' . DIRECTORY_SEPARATOR . 'config' . DIRECTORY_SEPARATOR . 'params-local.php'),
    require(__DIR__ . DIRECTORY_SEPARATOR . 'params.php'),
    require(__DIR__ . DIRECTORY_SEPARATOR . 'params-local.php')
);

return array(
    'id' => 'app-backend',
    'basePath' => dirname(__DIR__),
    'vendorPath' => dirname(dirname(__DIR__)) . DIRECTORY_SEPARATOR . 'vendor',
creocoder commented 11 years ago

@klimov-paul, @samdark I mean use DIRECTORY_SEPARATOR only for path generation.

resurtm commented 11 years ago

@samdark, it could be:

$rootDir = __DIR__ . DS . '..' . DS . '..';

$params = array_merge(
    require($rootDir . DS . 'common' . DS . 'config' . DS . 'params.php'),
    require($rootDir . DS . 'common' . DS . 'config' . DS . 'params-local.php'),
    require(__DIR__ . DS . 'params.php'),
    require(__DIR__ . DS . 'params-local.php')
);

return array(
    'id' => 'app-backend',
    'basePath' => dirname(__DIR__),
    'vendorPath' => dirname(dirname(__DIR__)) . DS . 'vendor',
slavcodev commented 11 years ago

I dislike idea to introduce another constant. Or use DIRECTORY_SEPARATOR or leave it as is.

samdark commented 11 years ago

Still it's less readable and doesn't really provide any benefits.