r72858 MediaWiki - Code Review archive

You don't need to include bootstrap, it's included when you run phpunit because it's defined in suite.xml.

I know, but that's not wrong either (just overly verbose) and keeps the script checking class existance happy.

Don't rely on include_path, you never know what it might be set to.

The default LocalSettings.php only adds "$IP/includes", not "$IP/includes/api", so here you're relying on undocumented behaviour in PHP which causes the directory of the currently executing script to be added to the end of the include_path. Not to the start, to the end. And the default LocalSettings.php only prepends to include_path, it doesn't replace it. This means that if any of the directories in the system default include_path have a file called ApiSetup.php, that file will be used instead of the MediaWiki one.

Use require_once(dirname(__FILE__).'/ApiSetup.php'), or better still, use the autoloader.

I had followed the example of ApiTest.php. Fixed in r73064.

My fault, I put that there when I reorganized things. I think adding this stuff to Autoload.php is keeping dependency information too far from the dependent files. Either append $wgAutoloadClasses in bootstrap.php or include things directly -- the later of which is my preference in this case, since shared code is very limited and would be more visible. Either way, it won't hurt to add dirname(__FILE__) . '/' if you are including directly, but documented or not, it seems like PHP has supported this functionality for a long time and is unlikely to pull it - all in all, we could also just cross this imaginary bridge when we come to it.

The problem with direct inclusion is that people keep introducing security vulnerabilities with it, or messing it up in other ways. The autoloader is relatively foolproof.

Fair enough. How do you feel about appending the autoloader in bootstap.php, or something else local to the tests?

It could be at maintenance/tests/phpunit/includes/Autoloader.php

resolved in r79443