Not automatically using the autoloader in GlobalState.php during class_exists #754

Closed
wants to merge 1 commit into
from

Conversation

8 participants
@brobie

brobie commented Dec 17, 2012

We found this need because:

  1. We do not have PHP_Invoker installed.
  2. We have multiple autoloaders registered
  3. When the class_exists was checking for "PHP_Invoker" it was
    automatically trying to autoload it and failing in a way that fatally
    ended our phpunit tests.
Setting the "use autoloader" switch to false on class_exists
We found this need because:
1. We do now have PHP_Invoker installed.
2. We have multiple autoloaders registered
3. When the class_exists was checking for "PHP_Invoker" it was
automatically trying to autoload it and failing in a way that fatally
ended our phpunit tests.
@edorian

This comment has been minimized.

Show comment Hide comment
@edorian

edorian Dec 19, 2012

Contributor

While we don't grantee that PHPUnit never ever calls the autoloader and suggest to not have a autoloader that dies when it doesn't find a file this case seems like something that is avoidable. I'm not sure when those files are loaded but I assume it's before code coverage and in that case everything that has been executed can be blacklisted and the rest can be safely ignored.

Thanks for the PR!

Contributor

edorian commented Dec 19, 2012

While we don't grantee that PHPUnit never ever calls the autoloader and suggest to not have a autoloader that dies when it doesn't find a file this case seems like something that is avoidable. I'm not sure when those files are loaded but I assume it's before code coverage and in that case everything that has been executed can be blacklisted and the rest can be safely ignored.

Thanks for the PR!

@tohann

This comment has been minimized.

Show comment Hide comment
@tohann

tohann Dec 19, 2012

We're interested in this fix, too. Does that mean this pull request will be accepted?

tohann commented Dec 19, 2012

We're interested in this fix, too. Does that mean this pull request will be accepted?

@edorian

This comment has been minimized.

Show comment Hide comment
@edorian

edorian Dec 19, 2012

Contributor

@tohann I currently don't oversee the implications of this and don't have time to regression test it so my plan was to leave it for Sebastian to decide.

If it doesn't break anything or cause other issues I don't see why it shouldn't be merged. We already try to avoid autoloader calls where possible.

Contributor

edorian commented Dec 19, 2012

@tohann I currently don't oversee the implications of this and don't have time to regression test it so my plan was to leave it for Sebastian to decide.

If it doesn't break anything or cause other issues I don't see why it shouldn't be merged. We already try to avoid autoloader calls where possible.

@tohann

This comment has been minimized.

Show comment Hide comment
@tohann

tohann Jan 2, 2013

Cool, thanks for the update!

tohann commented Jan 2, 2013

Cool, thanks for the update!

@jasonevans1

This comment has been minimized.

Show comment Hide comment
@jasonevans1

jasonevans1 Feb 21, 2013

I am interested in having this pull request accepted too.

I am interested in having this pull request accepted too.

@simonrjones

This comment has been minimized.

Show comment Hide comment
@simonrjones

simonrjones Feb 21, 2013

I too would like to see this pull request accepted. I've been writing some phpunit tests and came across this issue when I'm using the spl autoloader to autoload classes.

thanks,
Simon

I too would like to see this pull request accepted. I've been writing some phpunit tests and came across this issue when I'm using the spl autoloader to autoload classes.

thanks,
Simon

@sebastianbergmann

This comment has been minimized.

Show comment Hide comment
@sebastianbergmann

sebastianbergmann Feb 21, 2013

Owner

I am against this. Autoloaders are stackable (as in: you can have more than one registered). An autoloader should do NOTHING when it is not responsible for a class.

I am against this. Autoloaders are stackable (as in: you can have more than one registered). An autoloader should do NOTHING when it is not responsible for a class.

@brobie brobie referenced this pull request in netz98/n98-magerun Apr 27, 2013

Closed

Make `phpunit/php-invoker` a non-required Package? #127

@GuillermoCerezo

This comment has been minimized.

Show comment Hide comment
@GuillermoCerezo

GuillermoCerezo May 14, 2013

Thanks for that line! It helped a lot with Yii + PHPUnit + Windows.

Thanks for that line! It helped a lot with Yii + PHPUnit + Windows.

@Ragazzo Ragazzo referenced this pull request in yiisoft/yii Aug 8, 2013

Closed

CTestCase PHPUnit 3.7.10 autoload fails #1907

@spencer-aerian

This comment has been minimized.

Show comment Hide comment
@spencer-aerian

spencer-aerian Mar 3, 2015

Come on Sebastian. Invoker is an optional package (according to the documentation) but you are making it a prerequisite. This is breaking lots of developers if its not loaded (and if you do load it you then have to muck about installing an optional php extension locally and on integration servers.)
I think the patch 'should be applied'.

Come on Sebastian. Invoker is an optional package (according to the documentation) but you are making it a prerequisite. This is breaking lots of developers if its not loaded (and if you do load it you then have to muck about installing an optional php extension locally and on integration servers.)
I think the patch 'should be applied'.

@sebastianbergmann

This comment has been minimized.

Show comment Hide comment
@sebastianbergmann

sebastianbergmann Mar 3, 2015

Owner

I agree that the current situation is not ideal and am open to suggestion for how to improve it. Messing with the autoloader, however, because of broken autoloaders (see my comment in #754 (comment)) is not an option.

Owner

sebastianbergmann commented Mar 3, 2015

I agree that the current situation is not ideal and am open to suggestion for how to improve it. Messing with the autoloader, however, because of broken autoloaders (see my comment in #754 (comment)) is not an option.

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