Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Inconsistent case sensitivity in Yii autoloader #599

Open
tom-- opened this Issue · 52 comments

9 participants

@tom--

Say I want to use use a widget in a view thus:

<?php
$this->widget('MyWidget');

And the definition is quite conventional:

<?php
class MyWidget extends CWidget {

}

If I save that class in a file called Mywidget.php then the widget works if it is on NTFS, FAT32 or (normal) HFS+.

Now I deploy on FreeBSD or Lunix or something with a case sensitive(CS) FS and my application is broken.

I regard this as a bug because the inconsistent case sensitivity is (I have observed hanging out on Freenode#yii) often not expected by the user and it is not documented in the Yii Class Ref. or Def. Guide.

It is not expected by the user because 1) spl_autoload is case insensitive (CI) regardless of FS and users bring that experience to Yii; 2) the new Yii user's experience developing on Windows or Mac is of a consistently CI autoloader throughout her learning, application development phase and quite possibly during test too. Deployment is not the best time to learn this crucial lesson.

After the second time I fell foul to this, I spent some time studying Yii's autoloader and decided that I cannot easily make a fix or workaround for myself. The least bad solution for me was to set up a partition or disk image with a CS FS and put my Yii projects on that.

So: Should Yii simply expect users to use a CS FS in dev or find some other SQA process to eliminate such case typos? If yes, how do they learn that they need to? If no, a change in Yii is needed.

Explaining the behavior in documentation would be good not it is not sufficient—it just turns a bug into a really bad feature.

I don't know what to suggest for Yii's autoloader. I think it would be good for Yii and its users if its default autoloader were either CI or CS regardless of FS. But I don't know if that would be a breaking change for existing Yii apps or if it would have some other unacceptable consequences.

Alternatively, Yii could perhaps be configured to throw a carefully worded exception in a CI environment upon detecting a case mismatch. If this were the default when YII_DEBUG is true, that would help users learn that they need to find some way to ensure that case mismatches are not present in their code.

@samdark
Owner

I think we can't implement case sensitivity in case of insensitive FS. Neither can we make everything case insensitive since it will require a lot more processing from class search algorithm.

@tom--

I can't argue with you about that, samdark.

What do you think of adding an optional check for case mismatch that is only performed if YII_DEBUG is true AND the optional check is explicitly configured for the app. And this check is turned on in a Gii generated webapp?

And how about adding a test to requirements/index.php? It would alert to the danger.

@samdark
Owner

What would this case mismatch check look like?

Not sure about adding this to requirements since it will display the same warning for both case sensitive and case insensitive situations.

@qiangxue
Owner

Checking case-sensitivity of file names is a very expensive operation because it involves scanning files under different directories. Instead of asking the framework to do this work, developers should form a good habit of naming the files and keep CS in mind at all time.

@qiangxue qiangxue closed this
@tom--

What would this case mismatch check look like?

Are you asking: what would the PHP code of such a check look like?

Not sure about adding this to requirements since it will display the same warning for both case sensitive and case insensitive situations.

Really? I imagined a test for a CI FS. If the FS is CI then a warning is displayed. If the FS is CS then no warning is displayed.

@schmunk42

We have this problem also very often. It simply happens from time to time for different reasons (eg. hotfixes, code generators, ...).

Would it be an option for us to use a slightly modified autoloader, what would be your advice on how to ensure this?

@tom--

Checking case-sensitivity of file names is a very expensive operation because it involves scanning files under different directories.

I accept the importance of a high performance autoloader and that making it always either CI or CS would have unacceptable impact. This is not what I propose.

A test such as

if (YII_DEBUG && Yii::app()->getCaseSensitiveAutoload() === true) {
}
/** @return bool Simulate a case sensitive autoloader */
public function getCaseSensitiveAutoload() {}

at each invocation of the autoloader is not very expensive (and YiiBase may cache the value of the VA so the getter runs only once per page load).

@qiangxue
Owner

I don't mean getCaseSensitiveAutoload() is expensive. I mean loading files in case-insensitive manner is expensive. While we can cache the results to speed up things, it unnecessarily adds complexity. IMO, this is going the wrong way since the code from the very beginning is wrong (because it doesn't follow the rule), and we are asking the framework to fix the error.

@qiangxue
Owner

A sanity checking script running offline might help reduce such mistakes. But asking a framework to fix such error during run time is not a good idea.

@schmunk42

It would be just a development setting, aren't assets also treated in another, more expensive way, when running in debug mode? A script minifier or a a less compiler would be similar examples.

I don't think we need this in the core, but @tom-- : Have you evaluated the possibility of creating something like a "CICSAutoloader Extension" for Yii?

@tom--

@qiangxue: I conceded that point when samdark made it 3 hours ago.

Since then all I'm asking for is an optionally configured mode (effective only when YII_DEBUG is also true) under which: on a CI FS, if line 412 of YiiBase.php loads a file but the file name on disk does not CS match the requested file, YiiBase::autoload throws an exception.

Documentation and some kind of lint or static code inspector would certainly help. I thought about how to implement such a script and decided that using a CS disk image was easier.

@tom--

@schmunk42 yes, I considered it and decided against. For one thing, I don't feel confident about writing such a thing. Second, even if I thought I could do a decent job, I don't like the idea of using my own autoloader for dev and test (all of which happens on systems that by default use CI FSs) and the Yii autoloader in production.

@lubosdz

totally agree with quiang - it's a developer's problem, not framework's. Nothing more to say:-)

@schmunk42

Yeah, but it's something which occurs in the framework.
Anyway, I would say we could throw these three parts together and get what we want:
1. Create a class with an autoload() function like https://github.com/yiisoft/yii/blob/master/framework/YiiBase.php#L388
2. Add the CS check where needed http://www.php.net/manual/en/function.file-exists.php#74159
3. Import autoloader http://www.scribd.com/doc/78113553/80/Customizing-the-Yii-autoloader
@tom-- let me know, if you want to elaborate this ...

@tom--

@lubosdz I respect your opinion and understand that a lot of people will hold it. I see it as akin to arguing that the strictest error checking options in PHP need not have been implemented because those are developers’ problems, not the language’s.

It is an ideological argument, not a pragmatic one.

PHP, for example, in its extreme error checking modes, does what the PHP developers feel it reasonably can do to make it easier for developers to write robust code. That is an example of a pragmatic approach.

As it stands, programs using Yii behave differently on different filesystems. Yii is thus not portable between CI and CS filesystems. This is, I feel, a loss both for Yii and for its users.

If Yii cannot reasonably help users avoid this slip by behaving consistently across filesystems then I accept that. This is what @qiangxue and @samdark have said. It would unacceptably affect performance of the autoloader. That is a pragmatic answer and I do not contest it.

But I remain to be convinced that an optional error checking mode (as I outlined yesterday) is unreasonable. That would be a big help and I think it can be done at negligible cost.

Yii Framework is responsible for this matter of portability. I am not quite ready to accept that “blame the user” is the only reasonable answer.

@tom--

I filed this feature request for PhpStorm yesterday: http://youtrack.jetbrains.com/issue/WI-10713?projectKey=WI

Please upvote it

@tom--

@schmunk42 That looks like a pretty good plan!

I cannot devote much time to it. I am behind on another OSS contribution (https://github.com/tom--/php-cs_random_bytes) commitment and already feel bad about that. I really don't want to give the impression that I can contribute a lot. I'll do what I can.

But I am certainly willing to test this autoloader by using it in my own projects.

@mdomba
Collaborator

@tom--
Why do you say:
As it stands, programs using Yii behave differently on different filesystems. Yii is thus not portable between CI and CS filesystems. This is, I feel, a loss both for Yii and for its user

This is not about Yii.. it's about the underlying OS and PHP.. windows is case insensitive, linux is case sensitive... thats the main difference... your request to fix this in Yii can be compared to a request to Linus Torvalds to add case insensitivity check to the linux kernel so that developers get notified of their mistakes :D

In any case IMHO a way to go with this as an extension... not as yii core...

@tom--

@mdomba Why do I say this?

Yii provides many wonderful abstractions that allow the user to ignore the differences between various underlying technologies. Providing such abstractions is one of the goals of various languages, frameworks and APIs. Indeed its one of the most common goals in SW design. And I imagine it is one of Yii's too.

There are countless examples. I'll mention just two that pertain to file systems. In recent versions of PHP its ok to use / to represent a directory separator and PHP looks after you on Windows. The other is the spl_autoloader which is case insensitive regardless of FS. They didn't have to do these things. They could have said that its a difference in underlying technology implementations and therefore not our responsibility. But they did not. Why?

So I believe the idea that an abstraction layer such as Yii might provide this particular FS technology abstraction is not in fact incoherent.

Does this answer your question?

I have, in the mean time, been persuaded that Yii's goal of providing a very high-performance autoloader is not technically compatible with the requirement to be case insensitive.

But I have not been persuaded that an optional CS filename check after a successful include in the autoloader, available only if YII_DEBUG is true, that throws an exception if there is a mismatch, is incompatible with any Yii goal.

At the same time it seems clear that the opportunity to make a case for this to the core team has passed. I blew it. So I should accept that the best @schmunk42 and I can do is, as you say, an extension.

@qiangxue
Owner

@tom: I think the current autoloader already throws an error when you try to include a nonexisting file due to CS issue.

@tom--

@qiangxue I am aware of a PHP warning if a PHP include() that should work does not for the reason you mention.

What I want is an exception on a CI FS when a file is successfully included by PHP but the name of the class requested does not CS match the file name.

The test would be disabled in runtime environments. It would allow typos to be detected before deployment.

@xwz
Collaborator

What I want is an exception on a CI FS when a file is successfully included by PHP but the name of the class requested does not CS match the file name.

I think this check is easy and cheap enough in debug mode, but should only check on the filename.

@mdomba
Collaborator

@tom-- I see your point and hopefully this could give developers more informative message then just "file not found" so they will not go like "how Yii cannot find this file when it's there"...

Can you try to implement this ?

@samdark
Owner

I think it's a good idea to add it but we should test if there's any significant performance difference and if it's acceptable.

@samdark samdark reopened this
@qiangxue
Owner

One solution is to use ReflectionClass to compare if the defined file name is the same as the class name. This requires creating a ReflectionClass object for every autoloaded class. I'm not sure if it is worthwhile to do so (note that this feature is only useful for users who work on non-CS file systems).

@samdark
Owner

@qiangxue non-CS filesystems aren't that rare. These are defaults for MacOS and Windows.

What we really want to know in this case is the fact that file we've autoloaded is named in a different case compared to the class name provided to the autoloader method. So we already have a class name and there's no need to use reflection.

@qiangxue
Owner

@samdark Could you use code to explain your solution?

@tom--

I am willing to take a crack at this. Even if I don't come up with the best way to do it, I should be able to make clear what the idea is.

@samdark
Owner

@qiang, the check itself will look like the following:

if(YII_DEBUG && $className.'.php' != basename($className.'.php'))
    throw new CException("Class name does not match file name.");
@qiangxue
Owner

This doesn't seem to be a solution as it has nothing to do with CS.

I just found another solution using realpath() which can automatically correct the case (tested on Windows, not sure about Mac):

basename(realpath($className . '.php'))  !== $className . '.php'
@tom--

@qiangxue this test works on OS X

@tom--

changing YiiBase.php:418 (http://code.google.com/p/yii/source/browse/tags/1.1.10/framework/YiiBase.php#418) to:

{
    include($className.'.php');
    if (YII_DEBUG && basename(realpath($className . '.php'))  !== $className . '.php')
        throw new CException("Class name does not match file name.");
}

In my test on a CI HFS+ partition, resulted in an exception while on a CS HFS+ partition it resulted in a PHP Warning. And that's what I was hoping for.

@tom--

I sent my ideas for it in a pull request

@samdark
Owner

@qiangxue I've actually meant realpath. For some reason removed it just before copy-pasting. Your latest one looks good to me. Will do some performance testing with and without it.

@qiangxue
Owner

The performance should be fine. realpath() is fast when testing an existing file. I did this test before.

@qiangxue qiangxue closed this
@qiangxue
Owner

I just checked in a fix. It turns out that we only need to do this check when relying on include_path to include a class file. Checking CS for other cases may cause problems in certain scenarios. To use this new feature, you have to set Yii::$enableIncludePath=true.

@manuel-84

today I deployed an application from a win64 enviroenment to a linux64 and i have this problem with zii.widgets.jui.CJuiDatepicker , the error is

Alias "zii.widgets.jui.CJuiDatepicker" is invalid. Make sure it points to an existing PHP file. :/

@samdark
Owner

@manuel-84 is it OK with release code?

@manuel-84

@samdark I can only say that I'm using yii-1.1.10.r3566, the last published on yiiframework.com

when calling widget('zii.widgets.jui.CJuiDatepicker') the app crash on linux, works in windows

@cebe
Owner

@manuel-84 it has to be CJuiDatePicker not CJuiDatepicker.

@samdark
Owner

@manuel-84 Can you try https://github.com/yiisoft/yii/zipball/master? It should give you warning under Windows.

@cebe
Owner

@samdark did you see my comment?

@manuel-84

@cebe that's really true, thanks

@samdark
Owner

@cebe Yes, I did. You have a solution. Moreover latest version should warn about this situation on Windows.

@manuel-84

@samdark I've manually added #bc9c6c1 , YII_DEBUG is true , logging 'levels' => 'error, warning, trace', I don't have any warning in application.log ... I'm missing something?

@samdark
Owner

@manuel-84 are you using code from master?

@manuel-84

@samdark yes I tried also with that.

@samdark samdark reopened this
@mdomba
Collaborator

@samdark

why reopen this issue - it's fixed for most uses, I don't see what else can we do here.

@manuel-84

The code from the master... did you try on the windows machine or on the linux machine?
This code should work on windows machine and notify you of a misplaced case... on linux machines you will still get classic errors .

@samdark
Owner

@mdomba I want to try reproducing it myself on Windows machine.

@samdark
Owner

Reproduced. Closing this one. Will add another that's more specific.

@samdark samdark closed this
@tom--

I welcome the test added to YiiBase::autoload but doesn't help on HFS+ CI—the default FS on Mac OS X and the only FS allowed on an OS X boot volume. I wrote about this previously in issue 605.

PHP realpath() does not reliably return the filename in the case in which it is cataloged on the FS. (1) this aspect of realpath's behavior on a CI FS is undefined. (2) it is not consistent over CI FSs.

I confirmed the current test works on Windows XP NTFS: it returns the file name in the case it has on the FS. But I was previously mistaken that it works on HFS+. For example:

$ php -r "var_dump(realpath('/uSr/BIN/phP'));"
string(12) "/uSr/BIN/phP"
$ php -r "var_dump(realpath('/usr/Local/BIN/phP'));"    # symlink to /usr/bin/php
string(12) "/usr/bin/php"

Calling PHP glob() on the filename is more predictable but still useless for the current purpose:

$ php -r "var_dump(glob('/uSr/BIN/phP'));"
array(1) {
  [0]=>
  string(12) "/uSr/BIN/phP"
}
$ php -r "var_dump(glob('/uSr/Local/BIN/phP'));"
array(1) {
  [0]=>
  string(18) "/uSr/Local/BIN/phP"
}

The best substitute I have come up with is something like:

in_array($file, $g = glob(dirname($file) . '/*'), true)

which functions appropriately.

To test its performance I produced a list of pathnames of 4652 regular files, chosen randomly, one per directory, from 4652 directories on my system. I ran the following test to compare glob with realpath:

<?php
`purge`; // force disk cache to be purged (flushed and emptied) Purge can be used to approximate initial boot conditions with a cold disk buffer cache for performance analysis.
$t0 = microtime(true);
foreach ($files as $file) {
    if (in_array($file, glob(dirname($file) . '/*', GLOB_NOSORT + GLOB_NOESCAPE), true))
    //if (basename(realpath($file)) === basename($file))
        $nPass++;
}
echo "$nPass " . (microtime(true) - $t0) . PHP_EOL;

The realpath took between 99ms and 102ms, 21.5µs per test.
The glob took between 265ms and 276ms, 58µs per test.
System was OSX 10.7.3, i5-2500K@4.3GHz DDR3-1600 OCZ Agility3.

On the same system but SW RAID-1 over two budget 3Gbps SATA HDDs: 21.5µs for realpath(same as on SSD!) and 82µs for glob.

Relative to realpath, glob on the dir is 2 to 3x slower. But in absolute terms in a YII_DEBUG environment, it wouldn't be a problem to me.

@samdark samdark reopened this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.