Release 3.2.1 breaks vendors behavior

[loic-couharde]

Hi,

The #66 hotfix breaks many vendors since Global arrays are cleared on setUp().
For example I use spatie/phpunit-snapshot-assertions to make snapshots which use $_SERVER here or Hoa Protocol here.
These are only 2 examples but it may have a huge impact on other large projects with many vendors.

Is it possible to consider fine fixing the problem encountered in #61 instead of clearing all global arrays ?

Thanks.

[Ocramius]

If this scenario needs to be supported, then we'd need:

• a failing test case (which passes before Hotfix #61 - Clear global arrays on setUp #66 was applied)
• a discussion (on the failing test PR) on whether the scenario is valid, and whether we want to go down that route.

Overall, changing $_ variables should not affect testing, and tests that expose failures if that happens are extremely smelly.

Specifically, in the two cases listed above:

• https://github.com/hoaproject/Protocol/blob/e2646429e37b757ef8c83503df6034030eafb4f5/Source/Protocol.php#L96 should be a ctor argument, or a named constructor. A protected function initialize() that is called post-construct isn't good design, and @Hywan may be able to help further in fixing that later on.
• https://github.com/spatie/phpunit-snapshot-assertions/blob/5430ae5ba2565d6b564386cf2a8140b8583ed5d6/src/MatchesSnapshots.php#L129 should be captured BEFORE booting up the test suite, since all variables in the runtime environment are potentially changed at runtime. Relying on a global at that point in time is incorrect/risky. This is something I'd report to @spatie directly as well.

[Ocramius]

As an added suggestion, consider using @runInSeparateProcess, if you have tests relying on super-globals or statics. I generally isolate anything that uses statics/globals (and all related anti-patterns) in such tests, at an extreme performance penalty, but usually works.

EDIT: this includes tests that rely on changing $_GLOBALS

[Hywan]

We can update hoa/protocol but we are certainly not the only ones doing so :-).

Thanks for the ping!

[reinfi]

This not even fails vendor products it fails all our integration controller tests now.

PHPUnit allows to add server variable via configuration (like HTTP_HOST i.e.). These configuration settings are now thrown away when ever a new controller test is started.

The next problem is, that if the test fails and throws an exception the global variables are not restored and phpunit now fails hard.

Is this really expected behaviour?

[michalbundyra]

Is this really expected behaviour?

No. I'll try to prepare another fix today, I have an idea and it could work for everyone! :)

[michalbundyra]

@reinfi @loic-couharde @Ocramius I've prepared PR:
#74 which depends on
zendframework/zend-http#165 - recognise correct base url.

[reinfi]

looks good to me

[jaydiablo]

We have some controller tests failing now with zend-test 3.2.1 (that includes changes from #66) because of this line in zend-session:

https://github.com/zendframework/zend-session/blob/master/src/AbstractContainer.php#L260

Exception 'PHPUnit\Framework\Error\Notice' with message 'Undefined index: REQUEST_TIME' in ./vendor/zendframework/zend-session/src/AbstractContainer.php:260

Reverting to 3.2.0 fixes the tests.