Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add a specific autoloader for case-sensitive OS #60

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Owner

michelf commented Jan 23, 2013

Are you trying to say that the Readme.php file currently doesn't work on case-sensitive filesystems?

I have a problem with this patch. The problem is that Readme.php is supposed to be a demonstration of how to use the library. However adding all this just to setup correctly autoloading breaks the balance as the biggest part of the sample code becomes about how to correctly setup autoloading.

chdemko commented Jan 23, 2013

Are you trying to say that the Readme.php file currently doesn't work on case-sensitive filesystems?

Yes it doesn't work here on linux. The other solution is to rename Michelf/Markdown.php to michelf/markdown.php

chdemko commented Jan 23, 2013

another solution is to replace

spl_autoload_register();

by

require 'Michelf/Markdown.php';
Owner

michelf commented Jan 23, 2013

I'd rather stay PSR-0-compliant, so lower-case filenames won't work. Using require_once would work, but I'd rather show people how to use autoloading, if only it wasn't so complicated. I'll have to think about this a little more and decide on a solution before the Lib branch goes out of beta. Thanks for letting me know of the issue, and don't hesitate to post again if you have other ideas.

@ghost Unknown commented on the diff Jan 23, 2013

Readme.php
@@ -5,7 +5,19 @@
# you like.
# Enable class autoloading
-spl_autoload_register();
+spl_autoload_register(function ($className) {
+ $className = ltrim($className, '\\');
+ $fileName = '';
+ $namespace = '';
+ if ($lastNsPos = strrpos($className, '\\')) {
+ $namespace = substr($className, 0, $lastNsPos);
+ $className = substr($className, $lastNsPos + 1);
+ $fileName = str_replace('\\', DIRECTORY_SEPARATOR, $namespace) . DIRECTORY_SEPARATOR;
+ }
+ $fileName .= str_replace('_', DIRECTORY_SEPARATOR, $className) . '.php';
+ @include $fileName;
@ghost

ghost Jan 23, 2013

Without giving any acceptance to this PR, this line is wrong and should be wrapped in a file_exists(). Use of @ to suppress errors like this is a well documented faux pas.

@chdemko

chdemko Jan 24, 2013

I have used the silent operator because the file_exists function adds a heavy time cost

@ghost Unknown commented on the diff Jan 23, 2013

Readme.php
@@ -5,7 +5,19 @@
# you like.
# Enable class autoloading
-spl_autoload_register();
+spl_autoload_register(function ($className) {
+ $className = ltrim($className, '\\');
@ghost

ghost Jan 23, 2013

Since this is a specific autoloader just for this library it should test to see if it should even run at all, e.g.

if (false === strpos($className, 'Michelf\\')) {
    return;
}
@ghost

ghost commented Jan 23, 2013

The Readme.php is simply some example code to show how it would be used. I would replace the autoloader line there with require __DIR__.'/vendor/autoload.php possibly wrap that up in a file_exists test and echo "please run composer install, see http://getcomposer.org"

@michelf - there is really no need for people to write autoloaders anymore. Composer has it, and there are plenty .autoloader libraries out there like Symfony ClassLoader component.

gheydon commented Jan 24, 2013

I have found one of the problems to do with this is that in the composer.json file the psr-0 line needs to also be case Michelf and not michelf

"autoload": { "psr-0": { "Michelf": "" } },
Owner

michelf commented Jan 24, 2013

Decided to use require_once instead of autoloading in Readme.php. This fixes this issue. f35f0c0

@michelf michelf closed this Jan 24, 2013

Owner

michelf commented Feb 1, 2013

Actually, I'll reopen this. Using require_once works when loading Michelf/Markdown.php, but it'll fail when loading Michelf/MarkdownExtra.php because the later assumes PSR-0 autoloading is in place.

Of course the Readme file doesn't use MarkdownExtra, but it's an example and it should be easy to swap one for the other.

So I guess might have to include a PSR-0 autoloader in the example. Perhaps it could be put in a separate file to keep the Readme file simple.

@michelf michelf reopened this Feb 1, 2013

Owner

michelf commented Feb 3, 2013

Now using custom-made autoloader that doesn't take too much space. That should fix the issue. ca799f8

@michelf michelf closed this Feb 3, 2013

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