Handling assert() statement failures in PHP 5 and 7.

[ghost]

The Drupal project has begun using runtime assertions (the PHP assert() statement) to supplement unit tests since they are idea for policing API interactions between disparate units of code. While assert() statements usually don't need unit tests (usually it's redundant, like writing a unit test to test a unit test) occasionally it is necessary. The following patch was applied to Drupal and elements of this will be useful for anyone using PHP Unit alongside assert().

http://cgit.drupalcode.org/drupal/commit/?id=5bb2889

The method in that patch file sets PHP 7's assert.exception flag to 1 so that it will throw exceptions, then adds an assert callback to PHP 5 to do the same. The end result is both versions of PHP throw an \AssertionError when an assert statement is failed, and unit tests can then look for such failures using the existing test annotation or setExpectedException functions. There is the unavoidable slight difference that PHP 5's \AssertionError extends from \Exception, where in PHP 7 it will extend from \Error.

Having this as a default behavior in PHPunit would be highly useful.

[sebastianbergmann]

I am not sure what you want me to do here, sorry.

[ghost]

Bit of clarity here - I'm referring mostly to PHP's inbuilt assert() statement http://www.php.net/assert/ , not to the assertions found in unit tests. In the Drupal documentation these assertions are referred to as "runtime assertions" since when they are active they are checked outside the unit tests. They exist as a supplement to unit tests by helping contrib authors call the API correctly when they are writing new code, something unit tests can't do.

PHP 7 made significant changes to assert(): https://github.com/tpunt/PHP7-Reference#expectations

Most of these are for the better, but they have to be turned on and they present a challenge in writing tests that can pass under PHP 5 or 7 without modification. This issue request centers on creating that compatibility.

PHP 5 by default assert() triggers an E_USER_WARNING error whenever its expression evaluates to FALSE. Any unit test needing to test if an assertion fails on cue would need to look for PHPUnit_Framework_Error_Warning under the current system. This works, but it's sloppy.

PHP 7 does the same by default, but it also allows assert() failures to be thrown. It does this when the assert.exception flag is set to true, and the throw is \AssertionError.

Drupal 8 executes the Handle::register code in the above quoted patch during its PHPUnit bootstrap to get PHP 5 to mimic PHP 7's exception behavior as closely as possible. What I am asking is for PHPUnit to do the same - under PHP 5 register an assert() callback that then throws an \AssertionError, under PHP 7 just set the assert.exception flag to true and let PHP do the throw itself.

From there, all unit tests can use either annotation or the setExpectedException function to look for \AssertionError. This handler needs to be attached before the bootstrap file is loaded so existing tests that already register assert callbacks will be able to overwrite this new one without any change to their code, making this a backwards compatible change.

[phlopsi]

To be honest, this is an issue with Drupal, not PHPUnit.

If Drupal really uses assert() in it's PHP 5 context in tests, but also claims PHP 7 compatibility, it should either replace those old assert()s with PHPUnit's assertTrue() or clearly state that assert.exception must not be set to 1. At least if someone wants to run the tests, because they will not pass because of the required backwards compatibility.
Drupal could also just set assert.exception to 0 in the PHPUnit bootstrap file, if the testsuite is executed in a PHP 7 environment.
The last thing that comes to my mind would be to use the @requires annotation, to limit certain tests to PHP 5. See "Skipping Tests using @requires" for more information, at least until all the assert()s have been replaced.

If, on the other hand, Drupal uses assert() in their normal codebase, then there is definitely no way that it should be tested for, because they're not interchangeable with exceptions; at least not if used correctly.

[ghost]

"If Drupal really uses assert() in it's PHP 5 context in tests, but also
claims PHP 7 compatibility, it should either replace those old assert()s
with PHPUnit's assertTrue()..."

Runtime assertions (use of the PHP assert() statement) are a supplement to
unit tests - not a replacement for them and certainly not something unit
tests can adequately replace. They lie within the code itself, not in the
PHPUnit test classes as assertTrue() and its kindred do.

In Drupal they exist mostly to supplement PHP Type hinting, particularly to
provide primitive type enforcement beyond what PHP 5 is capable of (such as
assert(is_string($arg)) ), and return type hinting which isn't supported in
PHP 5, as well as a few other checks for impossible situations that would
indicate bad code has been introduced to systems, and other sanity checks
that are computational expensive but redundant once development has been
concluded.

As I explained in depth to the Drupal team, Unit tests have the limitation
of only being able to check for the known scenarios outlined in their
tests. Runtime assertions, when turned on, check for the unknown scenarios,
testing code even as it is being written. They enforce the code's
expectations, testing interactions between units of code rather than the
units themselves which is better left to unit testing.

Now while it is true PHP 7 has assertions trigger errors for BC reasons,
they aren't especially testable in that state. It is far easier to work
with them as exceptions. Granted, it is rare to unit test to see if an
assertion is thrown, but it does happen. Writing such tests is easier if
the behavior is the same in PHP 5 and 7. That can be achieved by allowing
an error to be thrown, but that has the disadvantage of not being distinct
from other errors that might come up.

If, on the other hand, Drupal uses assert() in their normal codebase, then
there is definitely no way that it should be tested for, because they're
not interchangeable with exceptions; at least not if used correctly.

They are used in the Drupal codebase and the unit tests use the PHP 5
assert callback to convert them into AssertionError exceptions to ease the
writing of the few unit tests that care about them (mostly in the caching
system - legacy code that checked cache integrity was moved over to
assertions to verify them, but retaining the tests on that code was
desireable). I will assume that what you mean by "at least not if used
correctly" is that the code shouldn't catch an AssertionError() and branch
it's behavior accordingly. That is the case for main code. There are a
couple simpletest unit tests that do this since they are directly testing
whether assertions are failing when they should.

On Sun, Feb 7, 2016 at 12:47 AM, phlopsi notifications@github.com wrote:

To be honest, this is an issue with Drupal, not PHPUnit.

If Drupal really uses assert() in it's PHP 5 context in tests, but also
claims PHP 7 compatibility, it should either replace those old assert()s
with PHPUnit's assertTrue() or clearly state that assert.exception must
not be set to 1. At least if someone wants to run the tests, because
they will not pass because of the required backwards compatibility.
Drupal could also just set assert.exception to 0 in the PHPUnit bootstrap
file, if the testsuite is executed in a PHP 7 environment.
The last thing that comes to my mind would be to use the @requires
annotation, to limit certain tests to PHP 5. See "Skipping Tests using
@requires
https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#incomplete-and-skipped-tests.skipping-tests-using-requires"
for more information, at least until all the assert()s have been replaced.

If, on the other hand, Drupal uses assert() in their normal codebase,
then there is definitely no way that it should be tested for, because
they're not interchangeable with exceptions; at least not if used correctly.

—
Reply to this email directly or view it on GitHub
#1897 (comment)
.

[phlopsi]

Thanks for clarifying what Drupal uses assert() for in detail. I'll try to respond to all of your mentioned use cases.

In Drupal they exist mostly to supplement PHP Type hinting, particularly to provide primitive type enforcement [...]

Type checking

You should throw an InvalidArgumentException instead of using assert(). Checking for a primitive type via is_*() is not an expensive operation. This would be a case of discussing typical micro optimizations, like die() vs exit or is_null() vs null ===. In this case though, InvalidArgumentException was specifically made for this, while assert() was not.

Also, If you use PHP's integrated parameter type checking for anything, I want to make clear, that nothing of it is checked at compile time, so you could argue that you would have to get rid of every possible type checking and instead use assert(), just to get that negligable amount of performance. If you plan to use PHP 7 primitive parameter type checks in the future, I can garantuee you that it is slower than not type checking at all, which is what assert() is doing in production, because it is not opcode cached unless configured otherwise.

Domain checking

Aside from type checking, there is also domain checking. While type checking should always be done where applicable, because it is inexpensive, you may decide to not check the domain of a provided value, that is, validating it's correct state, because this can indeed be expensive to do, e.g. validating a JSON-string.

While it seems doable to use assert() in this case, this is not what you want. If you have to decide whether to validate the domain or not, you are in 1 of 2 possible situations:

The code, where you have to make the decision, is [...]

1. only called internally
2. called externally, e.g. through your specified API

Situation 1: You do not need to check the domain in your code, because you have to trust your own/project's code. How can you trust that? Write a testsuite for the calling code, so you can be sure, it works as intended.

Situation 2: You must check the domain, because you cannot trust external code to work as intended. If you come to a point where you would have to check the domain multiple times and the domain check is expensive, you need a way to cache the validation result in some way. If you work with an array, string or another non-object you should probably introduce a wrapper class.
An instance of that class is either garantueed to represent valid state (validation "cached" through it's contract; created through factory, that checks the domain) or has other ways to be validated after creation.
Validating the object after creation can happen in 2 ways:

1. It's own validation method, which can be called explicitly e.g. $obj->isValid() or implicitly when executing one of it's methods
2. Let a designated validator class handle the validation, e.g. $validator->isValid($obj)

Since we, from time to time, tend to forget about doing things explicitly (thanks, human brain), having the object represent valid state is preferred, if possible. This also results in less type checking code, inreasing overall readability.

Closing words

I hope my arguments are clear enough to understand and sound reasonable to you, because this is what I've learned from experience over the years about that topic. If you have questions, if you think I have missed something or if you've made different experiences and want to share your opinion and/or knowledge with me, feel free to ask/tell me :)

I'll write more comments to get into detail about the other things, because maybe you can already respond to this in the meantime and I don't want to wreck anyone's brain by a gigantic text wall

[phlopsi]

[...] and return type hinting [...]

In my opinion, return type hinting as of now, is more or less just what it says, a hint. Your tests should already be checking for the correct type to be returned. A @return annotation is in many cases more flexible, because IDEs usually support static analysis to help you find possible type mismatches. The one advantage over tests and annotations it has, however, is the garantuee to always return the correct type or fail with an error. Not even tests can give that, because they can have bugs, too and we all love guarantees, because it's one less thing to worry about.

On the other hand, if you use assert() you have no such garantuee in production where it might make a difference and in a test environment the tests already cover that. All in all, I strongly argue, that assert() is useless in this case. If you feel the need to emulate PHP 7 return type hints, just throw UnexpectedValueException in case the type really is not the one you want. Though, I guess you only need this in really dynamic and complex functions/methods, which probably indicates, that you should refactor that part and split it into smaller and simpler chunks.

[ghost]

I'm not inclined to rehash the arguments I had over a six month period in
introducing assert() 's use to Drupal. Your points have been raised and
addressed before and the outcome of those debates is here.

https://www.drupal.org/node/2492225

Some additional points though.
"You should throw an InvalidArgumentException instead of using assert().
Checking for a primitive type via is__() is *not_ an expensive operation."

1. You've never validated a Drupal render array (which in a perfect world
   would be an object with setter calls to validate, but for legacy and
   performance reasons they are not). Adding assert() to verify them is one
   solution that prevents enormous overhead.
2. Primitive type checking isn't expensive until you do it 20,000 times
   because the function making the check is in a loop, at which point such
   quickly adds up.

2 Domain checking
I will not buy into your binary premise of only two situations in domain
checking. Situation #2 you outline above is meant for external services not
under the control of the developer. A Drupal module is not an external
service - it is an internal one - but while writing it mistakes can be made
even when unit testing is used. These modules get written by programmers of
all skill levels, including novices who don't write unit tests at all.
While assert() isn't going to be able to truly clear up their code, it can
alert them to when they are using Drupal core code incorrectly.

I respect your opinion greatly, but Test Driven design isn't a universal
panacea. Neither is Design by Contract - the approach assert() addresses. I
am convinced the best path for Drupal, a code base which must service
programmers across all levels of skill, includes the use of both
approaches. Unit testing helps insure the integrity of Drupal core. Design
by Contract exists, in Drupal at least, to police interactions with the
core by the modules.

On Sun, Feb 7, 2016 at 9:32 AM, phlopsi notifications@github.com wrote:

Thanks for clarifying what Drupal uses assert() for in detail. I'll try
to respond to all of your mentioned use cases.

In Drupal they exist mostly to supplement PHP Type hinting, particularly
to provide primitive type enforcement [...]

Type checking

You should throw an InvalidArgumentException instead of using assert().
Checking for a primitive type via is__() is *not_ an expensive operation.
This would be a case of discussing typical micro optimizations, like die()
vs exit or is_null() vs null ===. In this case though,
InvalidArgumentException was specifically made for this, while assert()
was not.

Also, If you use PHP's integrated parameter type checking for anything, I
want to make clear, that nothing of it is checked at compile time, so you
could argue that you would have to get rid of every possible type checking
and instead use assert(), just to get that negligable amount of
performance. If you plan to use PHP 7 primitive parameter type checks in
the future, I can garantuee you that it is slower than not type checking at
all, which is what assert() is doing in production, because it is not
opcode cached unless configured otherwise.
Domain checking

Aside from type checking, there is also domain checking. While type
checking should always be done where applicable, because it is
inexpensive, you may decide to not check the domain of a provided value,
that is, validating it's correct state, because this can indeed be
expensive to do, e.g. validating a JSON-string.

While it seems doable to use assert() in this case, this is not what you
want. If you have to decide whether to validate the domain or not, you are
in 1 of 2 possible situations:

The code, where you have to make the decision, is [...]

1. only called internally
2. called externally, e.g. through your specified API

Situation 1: You do not need to check the domain in your code, because you
have to trust your own/project's code. How can you trust that? Write a
testsuite for the calling code, so you can be sure, it works as intended.

Situation 2: You must check the domain, because you cannot trust
external code to work as intended. If you come to a point where you would
have to check the domain multiple times and the domain check is expensive,
you need a way to cache the validation result in some way. If you work with
an array, string or another non-object you should probably introduce a
wrapper class.
An instance of that class is either garantueed to represent valid state
(validation "cached" through it's contract; created through factory, that
checks the domain) or has other ways to be validated after creation.
Validating the object after creation can happen in 2 ways:

1. It's own validation method, which can be called explicitly e.g.
   $obj->isValid() or implicitly when executing one of it's methods
2. Let a designated validator class handle the validation, e.g.
   $validator->isValid($obj)

Since we, from time to time, tend to forget about doing things explicitly
(thanks, human brain), having the object represent valid state is
preferred, if possible. This also results in less type checking code,
inreasing overall readability.
Closing words

I hope my arguments are clear enough to understand and sound reasonable to
you, because this is what I've learned from experience over the years about
that topic. If you have questions, if you think I have missed something or
if you've made different experiences and want to share your opinion and/or
knowledge with me, feel free to ask/tell me :)

I'll write more comments to get into detail about the other things,
because maybe you can already respond to this in the meantime and I don't
want to wreck anyone's brain by a gigantic text wall

—
Reply to this email directly or view it on GitHub
#1897 (comment)
.

[phlopsi]

I'm definitely not familiar with Drupal, but render array + legacy, that's all I need to know. I've also read your linked post and the need for possible redundant validations, because of the legacy. I can only imagine this nightmare...

In short, it was untested code before and when trying to add validation at runtime, it would've killed the performance. In this case, I agree, assert() is much better than nothing at all.

Back to the original topic:
I just re-read your initial 2 comments and looked some things up.
As far as I can tell, this can break BC. See the following example testsuite, which currently works out of the box:

class WarningTest extends \PHPUnit_Framework_TestCase
{
    public function testWarning() {
        $this->expectException(PHPUnit_Framework_Error_Warning::class);
        $this->expectExceptionMessage('I am an assertion!');
        assert('false', 'I am an assertion!');
    }
}

If your requested feature would be implemented this test would fail, in both PHP 5 and 7, even if it ran without any issues before. It might even be the case that the project with this kind of test doesn't support PHP 7 yet, i.e. throwing a pseudo AssertionError might make no sense for them. Blame the legacy ;)

But this would only cover the case, in which this is the default. We could also make this configurable via something like --emulate-assertion-error. This would have the following effect (simplified):

if ($emulate_assertion_error) {
    if (PHP_MAJOR_VERSION === 5) {
        class AssertionError extends Exception
        {
            public function __construct($message, $code, $previous, $file, $line) { ... }
        }

        assert_options(ASSERT_WARNING, 0);
        assert_options(ASSERT_CALLBACK, function ($file, $line, $assertion, $description) {
            throw new AssertionError($description, 0, null, $file, $line);
        });
    } else {
        ini_set('assert.exception', 1);
    }
}

Something that came to my mind, after I've written all this (damnit) is, does this really benefit other people? If your application, e.g. Drupal, needs to transform the warning to an AssertionError, do you need this functionality in PHPUnit? Your application should already have all this logic, because it uses it, even if only in tests. You might be using more than just PHPUnit or something different entirely. You would still need to implement all this, because one of those test projects does not come with the ability to do this.

OT: Can Drupal modules edit the render array in some way, so refactoring woud be a very long process, touching a lot of code?

[sebastianbergmann]

I can only repeat what I wrote back in October: I do not understand what you want. Maybe a pull request could help.

[sebastianbergmann]

No feedback, closing.