processIsolation weirdness with constants in the bootstrap

[Tyrael]

today I noticed, that if I have a constant definition in my bootstrap file, the test execution will fail with an Constant xy already defined PHP Notice.

I stumbled upon this blogpost: http://css.dzone.com/news/process-isolation-phpunit-0 and I figured out, that phpunit tries to preserve the original global state for the isolated tests, but it seems that it does a little bit too much magic.

What I also found weird, that I also have includes in my bootstrap, which contains class declarations, and it seems that that won't trigger the "Cannot redeclare class" fatal error, so I don't really get it how exactly does the preservation works (didn't checked the source yet).

I created a github repo for the problem:

https://github.com/Tyrael/phpunit-processIsolation-bug

if you clone this repo, and run phpunit, it will fail with the Constant already defined error, if you change the processIsolation to "false" in the phpunit.xml.dist, then it will succeed.

[m4rw3r]

This is caused by the order of the items added to the template php-file which is assembled for launching the separate process.

Current order is:

1. Set include path
2. require_once PHPUnit autoloader
3. Define constants if not already defined
4. require_once for every file previously included (for some reason this also includes the bootstrap file)
5. Set used global variables
6. require_once for the bootstrap file, if configured

So if items 3 and 4 are swapped, it will fix this problem.

PHPUnit/Framework/Process/TestCaseMethod.tpl.dist contains the template with the order of the actions listed above.

The constants, globals and included files will be blank if the preserveGlobalState flag is set to false (don't remember if it will also include the bootstrap file then though). So that will probably also fix the problem, but the issue with the constants is still a bug imho.

[Tyrael]

swapping 3 and 4 seems logical, but including the bootstrap seems also unnecessary after doing that (including all of the included files, and restoring the global state)

Tyrael

[m4rw3r]

If you're interested in looking at the code which determines what to add and what not, here it is: https://github.com/sebastianbergmann/phpunit/blob/3.5/PHPUnit/Framework/TestCase.php#L526

[aaronromeo]

I've also run into this issue. I'm using processIsolation="true" in my phpunit config file. On my AWS machine I get a constant definition error.

RuntimeException: PHP Notice: Constant {SOME CONSTANT} already defined in {File name} on line {line no}

It appears that the bootstrap is loaded multiple times, and the constants are not purged in the tear down.

What is off about this is that the error is only thrown on the AWS machine. My local machine runs through this without an issue. This has me believing it is an environment setup issue.

However I cannot figure out what is different.

From my AWS EC2 machine

$ php -version
PHP 5.3.5-1ubuntu7.7 with Suhosin-Patch (cli) (built: Feb 11 2012 06:22:51) 
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies
    with Xdebug v2.1.3, Copyright (c) 2002-2012, by Derick Rethans
$ phpunit --version
PHPUnit 3.6.10 by Sebastian Bergmann.```

While my local machine has the following setup

$ php -version PHP 5.3.6 (cli) (built: Sep 15 2011 11:22:25) Copyright (c) 1997-2011 The PHP Group Zend Engine v2.3.0, Copyright (c) 1998-2011 Zend Technologies with XCache v1.3.1, Copyright (c) 2005-2010, by mOo with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans $ phpunit --version PHPUnit 3.6.10 by Sebastian Bergmann.```

I have wrapped most of my constants in defined() tests, but I would appreciate knowing if there is a cleaner option.

Thanks.

[whatthejeff]

I ran into this while working on b9ea952 and d2fac98. Seems it changed in PHPUnit 3.4.6.

Here's an excerpt from the changelog:

Fixed TRAC-942: Process Isolation should define constants before including files.

Unfortunately, I can't seem to track back the original issue. Maybe @sebastianbergmann has some recollection of why this change was needed.

[algimantas]

I get this for phpunit constants also:

Constant PHPUnit_MAIN_METHOD already defined...
Constant PHPUNIT_COMPOSER_INSTALL already defined...

[mohnishbasha]

Is this fixed or is there a plan to fix and roll out in future version ? I am also facing this issue with Constants.

[sun]

tl;dr: The current process isolation template conflicts with modern PHP programming practices. We can overhaul it in many small, possibly backwards-compatible steps, or we can replace it with a completely revamped template, targeting the next major release (sadly, 4.x is out already), but ASAP. Fixing this and related bugs is mostly a matter of BC and release management. The goal and user expectation is clear, but before anyone is able to work on anything, a strategy needs to be defined. Project maintainers will raise their voice, but a change like this affects 99.9% of all tests in the wild, so better think twice and share your thoughts on how we can possibly achieve this without breaking YOUR tests.

---

Based on my recent debugging work, it's fairly clear to me that the process isolation script template (+ injected data) requires a major overhaul.

The current template + data processing seems to originate from a time in which procedural code was the state of art, and most projects either did not need a bootstrap file or they used it to prime tons of global state constructs in unholy ways. — Do you remember good ol' times?

The current template nicely resembles PHP4 programming practices: (1) php.ini, (2) user-space constants, (3) include files, (4) global variables, and only after futzing with all that global state, (5) boot and execute your test.

That made total sense. Back then. :wink:

Now, let me go out on a limb and say: Today, for modern PHP5 code, the bootstrap file is exclusively used to (1) load + register a class loader and (2) register your namespaces.

In turn, the only job of the process isolation template is to boot your bootstrap file. Everything else is handled differently; to reiterate above PHP4 list:

1. Modern PHP5 code doesn't rely on PHP INI settings. Backing up + injecting the total state of INI settings into the child process is obsolete. If your code requires custom INI settings, then your bootstrap file will set them anyway already. (cf. #1343)
2. Constants are defined in interfaces and classes. Legacy constants are defined by legacy include files (see below). Defining constants ahead of time is obsolete.
3. Legacy/procedural include files are loaded by your bootstrap file (if they need to be loaded at all). Loading them ahead of time is obsolete.
4. Global variables are gone. If your code relies on custom globals, your bootstrap file will set them anyway already. Setting them ahead of time is obsolete.
5. Every test, regardless of whether in isolation or not, does exactly two things: Execute the bootstrap file, and execute the test.

→ The process isolation environment priming script template is reduced to (5).

Every test process is expected to boot from a clean slate, just like the initial/original phpunit process itself. Your bootstrap file adds a class loader, so your test class is found. It may also configure INI settings or even prime global state, but the bootstrap file is the single and only entry point for all tests.

Give or take #1343, you can achieve that modern, desired process isolation environment by undoing most of the global state backup in the base class of your tests (you should have one, regardless of whether you currently need one) like this:

  protected $preserveGlobalState = FALSE;

  protected function prepareTemplate(\Text_Template $template) {
    $template->setVar(array(
      'iniSettings' => '',
      'constants' => '',
      'included_files' => '',
      'globals' => '$GLOBALS[\'__PHPUNIT_BOOTSTRAP\'] = ' . var_export($GLOBALS['__PHPUNIT_BOOTSTRAP'], TRUE) . ";\n",
    ));
  }

This effectively force-reduces the process isolation template to the max: the bootstrap file.

The current state of the to be executed test is captured and forwarded in separate template variables, so no worries, your isolated tests will still execute correctly.

For modern PHP5 code, this matters in multiple ways:

1. There is no global state.
   • In order to inject global state, you do so explicitly.
   • Template variables allow you to forward desired data to the child process. Your bootstrap file consumes them.
   • Let's have dedicated ways for doing so in the framework.
2. Customized INI settings must not leak.
   • Currently, the global state of INI settings of the parent process leaks into child processes, whatever those INI settings may be, and regardless of whether a previous test method temporarily adjusted them. — Sorry, that's wrong. INI settings are backed up + restored for every single test run, even. Always, regardless of process isolation.
   • If you're relying on any INI settings, your bootstrap file has to re-set all mission-critical INI settings to guarantee reliable test results.
3. Global constants are defined by your legacy code, but only if and when that code is loaded.
   • Defining global constants upfront causes all of your legacy code to trigger PHP errors upon getting loaded, causing test failures.
   • Wrapping all of your legacy global constant definitions into a if (!defined('MY_CONSTANT')) does not make any sense.

4. The backup of included files loads your bootstrap file before loading the class loader, which may cause your class loader to get loaded twice.

   require_once 'tests\bootstrap.php';
   require_once 'vendor\autoload.php';

5. The backup of includes files can load up to hundreds (or even thousands) of classes that are irrelevant for the executed test. The backup simply contains each and every file that your class loader has loaded up until the process isolation for a particular test gets triggered. This typically means your whole application code + most of its dependencies.
   • Loading code consumes (potentially megabytes of) memory, and depending on your filesystem/hardware, loading many individual files may be slow. Avoiding to load code that isn't needed is the whole purpose of class loaders.

6. The backup of global variables contains a complete backup of PHP super-globals, which your modern PHP5 code avoids and ignores anyway:

   ...
   $GLOBALS['_SERVER']['HOME'] = 'c:/Users/sun';
   $GLOBALS['_SERVER']['PROMPT'] = '$P$G';
   $GLOBALS['_SERVER']['TMP'] = 'C:/Users/sun/AppData/Local/Temp';
   ...

7. The backup of global variables contains a (complete) backup of all variables that happen to be defined in your bootstrap file - which may or may not disrupt the global state of your tests in isolation:

   $loader = require __DIR__ . '/../vendor/autoload.php';

   $GLOBALS['loader'] = unserialize('O:29:"Composer\\Autoload\\ClassLoader":7:{s:48:"' . "\0" . 'Composer\\Autoload\\ClassLoader' . "\0" ......

8. I could go on, but omitting the rest for brevity...

Now, every modern project with reasonably complex tests needs to figure out the entirety of this on its own. The vast majority of users most likely isn't even aware of some of the issues listed above. (I wasn't either, until I had to deep-dive into the code in order to fix other process isolation bugs.)

Likewise, the fact that $preserveGlobalState is enabled by default definitely appears to trip up pretty much every new user.

The stated bug(s) in this and related issues are just simply symptoms of a PHP4-era process isolation template architecture. It probably worked fine for years, and personally, I even had a very positive "WOW" moment when I initially discovered this mechanism. However, it had it's time, its time is over.

The question is how to get from status quo to a completely revised process isolation architecture? There are two options:

1. Fix individual issues (like this) step by step. This focuses on backwards-compatibility; i.e., incremental fixes may be backportable to stable release branches. Every single change to the process isolation template needs to account for the possibility of custom templates (like the very hack I'm suggesting above).
2. Replace the template with a completely new one. Potentially remove all the code responsible for backing up stuff that is obsolete. Obviously, this ain't backportable and targets a new major release (or minor; PHPUnit doesn't really seem to follow strict SemVer). This effectively means that process isolation starts from scratch, and your current tests may break.
   • BC is hard, because literally everything extends from PHPUnit_Framework_TestCase, but just as an idea:
     Normally this is the domain of interfaces, but it may be possible to generally address the topic of BC by introducing version-specific code paths specifically for process isolation; i.e., if you assume the current version is 1.0, your current prepareTemplate() method would be invoked as-is; but if your class implements prepareTemplate2() instead, we're going to hook up your isolated test process using the new model. Better ideas welcome ;)

Long story short:

The question isn't whether there are bugs, the question is how we can fix them without breaking all of your tests. :wink:

[whatthejeff]

@sun I don't have the time to respond in detail at the moment, but keep in mind that one of the main purposes of the process isolation feature is to give people an avenue for testing legacy/hard-to-test code.

[sebastianbergmann]

What @whatthejeff said. In a world such as the one you describe, @sun, there would probably be no need for process isolation to begin with.

[bibyzhang]

I also encounter this question: PHP Notice: Constant COMPILER_HALT_OFFSET already defined PHP Notice: Constant PHPUNIT_PHAR already defined PHP Notice: Constant PHPUNIT_PHAR_ROOT already defined And my version of php and phpunit is PHP 5.3.10-1ubuntu3.6 with Suhosin-Patch (cli) (built: Mar 11 2013 14:31:48) PHPUnit 4.3.5 by Sebastian Bergmann.

[sebastianbergmann]

Dear contributor,

let me start by apologizing for not commenting and/or working on the issue you have reported or merging the pull request you have sent sooner.

PHPUnit 5.0 was released today. And today I am closing all open bug reports and pull requests for PHPUnit and its dependencies that I maintain. Please do not interpret the closing of this ticket as an insult or a lack of interest in your problem. I am sorry for any inconvenience this may cause.

If the topic of this ticket is still relevant then please open a new ticket or send a new pull request. If your ticket or pull request is about a defect then please check whether the issue still exists in PHPUnit 4.8 (which will received bug fixes until August 2016). If your ticket or pull request is about a new feature then please port your patch PHPUnit 5.0 before sending a new pull request.

I hope that today's extreme backlog grooming will allow me to respond to bug reports and pull requests in a more timely manner in the future.

Thank you for your understanding, Sebastian