Skip to content
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

The new autoloader breaks some frameworks with custom autoloaders #1333

Closed
bgaillard opened this issue Feb 3, 2017 · 4 comments
Closed

The new autoloader breaks some frameworks with custom autoloaders #1333

bgaillard opened this issue Feb 3, 2017 · 4 comments

Comments

@bgaillard
Copy link

bgaillard commented Feb 3, 2017

Hi, we are using Go-Aop (https://github.com/goaop) to manage database transactions in our source code.

Our AOP aspects should be executed inside our unit and integrations tests, Go-Aop is using its own Autoloader to implement its "weaving".

Since we updated to the last version of PHP_CodeSniffer our transactions do not work anymore because PHP_CodeSniffer defines its own autoloader. This disables the Go-Aop autoloader.

The code in Go-Aop which replaces the Composer autoloader is here https://github.com/goaop/framework/blob/master/src/Instrument/ClassLoading/AopComposerLoader.php#L107.

In their implementation the Composer autoloader is replaced by the Go-Aop autoloader, but now Go-Aop does not work because it cannot detect any Composer autoloader.

This is because PHP_CodeSniffer unplugged it here https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/autoload.php#L66.

Ideally it would be great to allow Go-Aop to detect that the autoloader in use is the PHP_CodeSniffer autoloader or vice versa.

But other frameworks could define their own autoloaders, in my opinion we should have a mecanism to disable the PHP_CodeSniffer autoloader while we run unit tests.

I'm thinking about 2 solutions.

Add a global variable to disable the PHP_CodeSniffer autoloader
Replace https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/autoload.php#L239 by the following peace of code.

    if(!(defined('DISABLE_PHP_CODE_SNIFFER_AUTOLOADER') && DISABLE_PHP_CODE_SNIFFER_AUTOLOADER)) {

        spl_autoload_register([__NAMESPACE__.'\Autoload', 'load'], true, true);

    }

Then we simply have to add the following instruction in our PHPUnit bootstrap code to disable the PHP_CodeSniffer autoloader.

// Disables the PHP_CodeSniffer autoloader because its not useful to execute unit tests
define('DISABLE_PHP_CODE_SNIFFER_AUTOLOADER', true);

Better

Simply remove https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/composer.json#L32 and load the PHP_CodeSniffer autoloader only from the CLI scripts to not have any impact on unit tests.

What's your opinion about this feature ?

@gsherwood
Copy link
Member

If PHPCS is being run, it's autoloader must be used or else it just wont work properly. If it is not being run, it's autoloader really shouldn't be loaded up at all because there is no need for it to know about autoloading of classes.

I don't think this is something you should have to worry about, so I don't think a constant is needed here. But obviously something needs to change if it is breaking projects with custom autoloaders.

I'll take a look and see if I can just not include PHPCS in the composer autoloader and still get it working in the code.

@gsherwood
Copy link
Member

I've removed the autoload lines from the composer.json file because I don't think they are needed - the PHPCS autoloader should just be included manually.

I've also change the PHPCS autoloader to re-register the composer autoloader instead of just unloading it. I need to unregister to ensure the PHPCS autoloader comes first, but I can re-register it after that with no problems. That change alone may have fixed this issue for you, if you'd like to test it out and see.

I've done the testing that I can from this end, so it would be really helpful if you were able to get the latest 3.0 dev code and test again.

@bgaillard
Copy link
Author

bgaillard commented Feb 7, 2017

Hi Greg, thanks for your reactivity on this case.

I tested again with the 3.0.0RC3 and 3.0.x-dev versions and I can confirm it is no working with version 3.0.0RC3 and it is working with 3.0.x-dev.

So I think your fix is valid (and you can close this case if its ok for you).

@gsherwood gsherwood changed the title The new Autoloader breaks Go-Aop and possibly other frameworks with custom autoloaders The new autoloader breaks some frameworks with custom autoloaders Feb 7, 2017
@gsherwood
Copy link
Member

Thanks a lot for testing that for me. I really appreciate it.

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

No branches or pull requests

2 participants