theseer / Autoload

A lightweight php namespace aware autoload generator and phar archive builder
Other
388 stars 47 forks source link

class redeclaration with conditional classes #49

Closed glensc closed 10 years ago

glensc commented 10 years ago
+ cd HttpFoundation-2.4.8
+ phpab -n -e '*/Tests/*' -o autoload.php .
phpab 1.14.2 - Copyright (C) 2009 - 2014 by Arne Blankerts

Scanning directory .

Error while processing request:
 - Redeclaration of class, interface or trait 'Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeSessionHandler' detected
 Original: ./Session/Storage/Handler/NativeSessionHandler.php
 Secondary: ./Session/Storage/Handler/NativeSessionHandler.php

reality is that the class is defined on condition in NativeSessionHandler.php:

if (version_compare(phpversion(), '5.4.0', '>=')) {
    class NativeSessionHandler extends \SessionHandler {}
} else {
    class NativeSessionHandler {}
}

ps: it would be also nice if phpab would understand the error is in the same file (and say accordingly), as i had to stare the error for a minute to find what's the difference in two filenames :)

theseer commented 10 years ago

I'll be looking into enhancing the error message. Feature wise you are looking for --tolerant.

theseer commented 10 years ago

On a side note: Conditional class definitions are a bad design choice and thus not "enabled" by default. Classes should not be defined conditionally but only loaded conditionally - implementing a common interface.

glensc commented 10 years ago

yes, i used --tolerant, but it's global option, not per file, so if i enable it, i could be missing some other conflicts.

also: warning about duplicate class in same file is kind of pointless: reading "either file" will get the class be loaded :)

theseer commented 10 years ago

Yes, the --tolerant switch is a global option. I do not see a point in limiting it to certain files only. It will only accept duplicate classes in the same file, which is, as you already mentioned, not really a problem from the autoloading perspective but a bad (imho broken) design choice.

I merely added the switch to support (imho broken) frameworks that use them since it doesn't break autoloading per se. Having different dependencies per implementation will affect static list generation as - due to the nature of a classmap - resolving dependencies with duplicate classnames doesn't work reliably. (How am I to resolve a foo extends bar if there is no way of telling which implementation of bar is being used and thus which of its potential dependencies should be loaded first?)

I don't think there is any change required except for the error message.

glensc commented 10 years ago

I think projects which have same class in different files exist too, possibly they did not have autoloader when first designed, and would be good if such cases are reported too, composer for example just warns in such case when same class is defined in more than one file, and states that only first occurrence will be actually used.

theseer commented 10 years ago

Yes, those projects do exists and code like that is not compatible with autoloading. Nothing that can be done about that except rewriting the software or fixing the names. The Autoload builder cannot produce any sane results for such a code base. If Composer's behavior is as you say, it is broken beyond any repair since the generated map would be absolutely useless and results would be unreliable at best.