Remove all calls to assert_options() conforming to PHP 7 spec, and adjust all runtime assertion related unit tests to skip on assert off.

I have adjusted the summary to account for PHPUnit's command line behavior. It isn't going to be possible to rely on .htaccess to force assert.exceptions to be set - it will be incumbent on the server settings.

I have a working patch. There are still 2 assert_option() calls to set assert.exception to 1, otherwise all assertion related unit tests will fail. These lines appear in PHPUnit's bootstrap.php and the Simpletest TestBase class. They are marked for removal as soon as the test runners have had their settings updated as called for in a child ticket. This patch doesn't contain the status report modification - It's already getting big and I think it would be best to do the status report change in a child issue.

I've also updated the issue summary to more accurately describe what is in the patch.

PHP 5 breaks are arising out of my emergency ASSERT_EXCEPTION constant shim. Once the unit testers are updated with assert.exception set to 1 I'll be able to reroll the patch with those two lines removed and get a clean PHP 5 run.

PHP 5.6 and 7.0 are running clean. Also had to change all zend_assertions references to zend.assertions. PHP and consistency...

This patch still needs the test runners to be updated to set assert.exception to 1 on their own - then I can drop the lines that are currently doing this.

I have added the following issue to the PHPUnit github as there's no way I know at present to handle test skipping when assertions are off, but there needs to be:

https://github.com/sebastianbergmann/phpunit/issues/2846

The recommendation I put forward would allow us to use an annotation of

@requires setting zend.assertions 1

If accepted we may have other use cases involving other PHP settings.

I'll get a patch reroll addressing the nits later today :)

With the parent being committed this issue comes to the front.

Question: Should I split the documentation part of this ticket off to another so that it can be folded in? Reason - Until this ticket is added the API documentation regarding runtime assertions is now slightly wrong.

This ticket also needs the test runners to be updated so that the patch can have the two stopgap assert_option() calls removed that are setting assert.exception to 1.

Bump. While not as urgent as some tickets, until this is applied the documentation on runtime assertions in Drupal will be incorrect.

Moving to critical as @neclimdul has encountered bugs arising from unit tests failing when zend.assertion is set to -1 and then they run anyway. The patch he composed addresses the same tests as this one (I think, I didn't do an exhaustive search, but there are at least 3 matches), a duplication of effort. This patch needs to be applied to prevent further duplication of effort and confusion.

EDIT: Also, for this patch to enter final phase the test runners must be set to assert.exception 1. Again, there is no existing test this will break as the current code base sets that value on its own using assert_options()

diff --git a/core/tests/Drupal/Tests/AssertHelperTrait.php b/core/tests/Drupal/Tests/AssertHelperTrait.php
index ba814d9641..1ea613c390 100644
+  protected function expectAssertion($message = '') {
+    if (ini_get('zend.assertions') != 1) {

This needs to be > 0 like all the others I think.

Otherwise this looks good to me.

As I mentioned in chat, I'm going to work on implementing annotation logic in PHPUnit, and that syntax should be our long term goal - but this is great for the interim I think. The inclusion of that logic in PHPUnit has been signed off on, so I just need to get a working pull request in place.

Could someone apply this interdiff to the patch at #18. I have a full patch, but jenkin's can't apply it because I'm on a windows machine and the long file names bug is rearing it's head. My Linux laptop that I normally do work for here on is currently down.

The interdiff instructs on how to get the patch to it's final form, removing the temp lines from the patch that were necessary to get it to run prior to the config change of the test runners. MixoLogic has applied that change, so the patch should now run without any temporary lines.

Well, at the end of the day your construction is closest to how PHPUnit is going to handle this for us. This is the relevant section of the pull request that was accepted this morning.

if (!empty($required['setting'])) {
    foreach ($required['setting'] as $setting => $value) {
        if (ini_get($setting) != $value) {
            $missing[] = \sprintf('Setting "%s" must be "%s".', $setting, $value);
        }
    }
}

Eventually tests will converted to have this annotation

@requires setting zend.assertions 1

So worrying about a conditional that we know will be pulled in a future patch after Drupal has a version of PHPUnit supporting this annotation doesn't help, and I'm sorry I brought it up.

Anyway, I can't do the re-roll because I can't get any patches done on my work machine to apply because of the Windows long filenames bug and my Linux laptop is down, So could someone else reroll the patch at #18 ignoring my comment at #19 because the PHPUnit change makes it moot. Two changes only are needed, this

diff --git a/core/modules/simpletest/src/TestBase.php b/core/modules/simpletest/src/TestBase.php
index 5d41735e5b..dfaac2c743 100644
--- a/core/modules/simpletest/src/TestBase.php
+++ b/core/modules/simpletest/src/TestBase.php
@@ -1392,8 +1392,3 @@ public function getTempFilesDirectory() {
   }
 
 }
-
-// THIS LINE MUST BE REMOVED AFTER TEST RUNNERS ARE RECONFIGURED!!!
-if (defined('ASSERT_EXCEPTION')) {
-  assert_options(ASSERT_EXCEPTION, 1);
-}

and this..

diff --git a/core/tests/bootstrap.php b/core/tests/bootstrap.php
index a92cb65ee0..ce3b45233d 100644
--- a/core/tests/bootstrap.php
+++ b/core/tests/bootstrap.php
@@ -161,11 +161,6 @@ function drupal_phpunit_populate_class_loader() {
 // reduce the fragility of the testing system in general.
 date_default_timezone_set('Australia/Sydney');
 
-// THIS LINE MUST BE REMOVED AFTER TEST RUNNERS ARE RECONFIGURED!!!
-if (defined('ASSERT_EXCEPTION')) {
-  assert_options(ASSERT_EXCEPTION, 1);
-}
-

The test runners have been reconfigured so these temporary lines should now be removed please.

The documentation changes in this are needed, but otherwise agree. I'll move work on getting those changes done to a child ticket.

This patch doesn't include the test skip logic. Are we deferring this?

No. All that was moved to the child was the documentation changes in core.api.php.

On point 1, that's actually a very good idea.

On point 2 - the "if you are using PHP 7.0" bit can be dropped because this ticket isn't going to be applied until PHP drops 5 support anyway. PHP ships with this set to 1, "on"; and in production it is strongly recommended that the setting be switched to -1. I didn't mention the later part because of the file this would appear in - settings.local.php is supposed to be a dev environment setup file yes?

That reaches out to the reason this needs some profiling done. There's a high chance that some users will be running their production machines with zend.assertion set to 1. That won't break anything, but it could slow it down and knowing by how much will help inform further decisions.

On priority - I pushed it up to critical at the same time I pushed it's parent to critical since at the time I thought they'd probably need to go out together. That isn't the case so the drop in priority makes sense and I just hadn't revisited it.

trigger_error('This function is deprecated. Please remove the call to it.', E_USER_DEPRECATED);

That should do the trick.