[policy, no patch] Define best practices for using and testing assertions and document them before adding assertions to core

Not certain on whether this should be postponed - cache optimizations still hinge on this being prepared, and I don't like the idea of launching with the services.yml problem that brought me here still being unresolved. That said..

We do not currently support them in our automated testing APIs.

Actually, this isn't true. Let me explain.

PHP ships with assert.active set TRUE. Our online test runners inherit this setting. They also have the PHP default setting of assert.warn set TRUE. This means that when a runtime assertion fails during a test an E_WARNING is triggered. PHPUnit stops the test immediately, Simpletest logs it in the exceptions log but proceeds - in both cases though the test is marked as failed.

#2408013: Adding Assertions to Drupal - Test Tools. changes the .htaccess setting for assert.active to 0 for Drupal's default. This means tests that use the web test runner (the functional tests) are running with assertions turned off. This is the main issue #2548671: [policy, no patch] Define best practices for using and testing assertions and document them before adding assertions to core looks to solve. At the same time it seeks to make PHP 5 and 7 do the same thing when an assertion fails - throw an AssertionError. With that in place making a unit test against a runtime assertion becomes possible.

So the automated testing APIs have always supported assert(), but the assert tools patch broke it to make Drupal behave in an optimized manner.

and a example.settings.local.php example for enabling them on a development environment.

To clarify, the example.settings.local.php example countermands the default .htaccess setting that was introduced. Unlike the other settings in .htaccess, assert options can be changed at runtime. This approach was done so that users who wish to activate runtime assertions can do so without editing .htaccess.

Now onto the main topic - when should assert be used, and when shouldn't it be? When should it be tested, and when shouldn't it? Here's my opinion on the matter. First up though, why are we using them? Well, do you want this function running in production??

  public function validateTokens(array $context_tokens = []) {
    if (empty($context_tokens)) {
      return;
    }

    // Initialize the set of valid context tokens with the container's contexts.
    if (!isset($this->validContextTokens)) {
      $this->validContextTokens = array_flip($this->contexts);
    }

    foreach ($context_tokens as $context_token) {
      if (!is_string($context_token)) {
        throw new \LogicException(sprintf('Cache contexts must be strings, %s given.', gettype($context_token)));
      }

      if (isset($this->validContextTokens[$context_token])) {
        continue;
      }

      // If it's a valid context token, then the ID must be stored in the set
      // of valid context tokens (since we initialized it with the list of cache
      // context IDs using the container). In case of an invalid context token,
      // throw an exception, otherwise cache it, including the parameter, to
      // minimize the amount of work in future ::validateContexts() calls.
      $context_id = $context_token;
      $colon_pos = strpos($context_id, ':');
      if ($colon_pos !== FALSE) {
        $context_id = substr($context_id, 0, $colon_pos);
      }
      if (isset($this->validContextTokens[$context_id])) {
        $this->validContextTokens[$context_token] = TRUE;
      }
      else {
        throw new \LogicException(sprintf('"%s" is not a valid cache context ID.', $context_id));
      }
    }
  }

This function is EXPENSIVE to run, early profiling showing that it and Cache::validTags() slow the system down by about 2%. Now imagine a similar function analyzing a render array's validity in Drupal\Core\Theme\Render::doRender(). A further 5% slowdown isn't unreasonable to imagine from that.

That's what assertions do - they analyze code state at runtime and verify whether or not the PHP docs are being followed or whether it's a bunch of wishful thinking. In what little time I've been poking around I've found that as almost as often as not that's exactly what they are - wishful thinking. Unit tests can only cover their own actions - they can't watch the runtime and guarantee the API is being followed.

When assert() must be used.

1. To check the scalar data types of all API methods that are specific about such arguments.

That said, avoid writing methods that are picky about their parameters - this is PHP not C#.

1. To check the data types of collections passed to public methods.

This is abbreviated in the PHP Docs as string[] or Some/Class[]. These show up a lot in Drupal, and they are what the Assert Tools were written to deal with.

When assert() may be used

1. To insure for unreachable code areas aren't reached.

Example

if ($a > 0) {
  return $b / $a
}
else if ( $a < 0 ) {
  return $c / $a
}
assert(FALSE);

The assertion fails when $a == 0. The PHP 7 assert RFC contains additional examples of this type of unreachable code branch asserting. Also, this is the one case where single quotes on the assertion aren't needed.

2. To check object state.

The most frequent example being checking to insure the service container has provided a dependency before it is called (this will not be true if a services.yml file is miswritten).

3. To check the return data type of a 3rd party library.

3. To check parameters on the methods that aren't part of the API.

When assert must not be used

1. To test any condition that could occur in production.

Assertions are for impossible conditions given correct code and configuration.

2. On the parameters of any Twig filter or function.

The chances of user submitted data hitting such is too high for assertion to be used reliably here.

3. Whenever there is doubt about #1.

Assertions and Unit Tests

Assert statements do not normally need unit tests. They are, after all, tests in their own right - testing an assert statement is akin to testing a unit test. If they are not met they will cause any and all unit tests where their code is being tested to fail. Sometimes though it is necessary

1. Conversion

If you are converting an existing exception throw to an assertion and there are existing unit tests of that exception throw then the tests usually need to be converted, not dropped.

2. Complex Analysis

If the assertion is doing a complex analysis like the CacheContextManager::validContext method then that will need to be tested.

3. Regex

If the assertion uses a regular expression testing is advised.

In no case should an assert statement in a protected method be unit tested - further it is usually bad practice to unit test protected methods to begin with - unit tests should deal with the code's outward facing API most of the time.

If you are testing whether an assert statement will actually fail on cue (resulting in an AssertionError) you can use PHPUnit's @expectedException annotation, or a try/catch block in Simpletest.

I missed the assertion commit (#2408013: Adding Assertions to Drupal - Test Tools.), just saw the change record this week. Setting active - like it or not, assertions are here and we need a clear published policy.

The eval aspect.

Well, the first thing to debate and probably get a vote on is the eval() issue.

Whenever a sting is passed as the first argument of assert() it will be evaluated as PHP code. I'll hazard a guess that this is the reason why other projects have avoided assert despite the testing features it brings to the table. The following is highly dangerous when $arg is an unsanitized string:

assert($arg);

During the building of the assert tools alex pott and I discussed this at length. The decision was made to proceed with due caution because the testing features are worth the risk. Let's look at those risks in detail. I constructed the following exploitable example.

function foo() {
	echo 'called';
}

$evil = "') && foo();//";

assert("is_string('$evil')");

echo 'end. evil  was: ' . "is_string('$evil')";

Single quotes stops the eval before eval and should be safe. Double quotes are dangerous, but what is the most likely mistake here?

assert("is_string($evil)");

Which won't run even with a valid string. Still, some programmers will jump to the exploitable example before doing the single quotes method.

There's also a legiblity concern - eval strings work oddly with the namespace directive. The see things like static, parent, self, $this, but they miss any use statements necessitating classes be addressed by their fully qualified names. Because of the questionable decision long ago to use \ as the namespace scope resolution operator this leads to lots and lots of escape sequences. Like:

assert('Drupal\Component\Assertion\Inspector::assertAllObjects($collection', \'\Foo\', \'\Bar\None\')');

Granted, in single quotes the only escape sequence is \' but PHP also allows \\ to be forgiving with those who forget that. It's a potential mess. And then there's the joy of regex.

Assert without eval

Assert doesn't have to take a string argument - it will take any argument and check it's truth value. Function arguments, expressions, operations, anything really. And these are likely safer and easier to read than eval strings but they come at the cost of being executed even when assertions are turned off. Since being able to disable these statements is one of the reasons for using assert() instead of normal control structures this approach is also problematic for PHP 5. In PHP 7, assert can be truly disabled at all times like in all other programming languages. This full disable at compile time directive though must be set in php.ini - so Drupal can't ship with the directive in place as it can with assert.active.

A possible compromise is explored in #2569049: Add a hook_requirements() that warns if assertions are turned on and discourage/remove double quotes in assert() - Have any function that is doing a potentially length assertion manually check the value of assert.active before executing - if it's set to FALSE then the function returns TRUE and doesn't run further, avoiding most of the performance loss. This has most of the same effect as turning assertions off entirely and does it in a forward compatible manner with PHP 7.

I'm leaning towards this compromise approach.

A big yes to single-quote eval from me. This will allow us to do important performance-sensitive error checks in often-used Drupal functions that would otherwise get refused out of performance concerns.

There's too many ifs and buts for this to be a real security issue. "Novice programmers" using asserts is a big "if" by itself. Me and @pfrenssen tried to exploit assert() and we could only do so when using used double-quoted asserts with the "assertions active" setting turned on in PHP.

As to double-quote eval, it should should just never be done under any circumstance as that's where there is a real security concern. This is something core committers can always check for in doing final patch reviews and is something we can build into CI as we implement CodeSniffer over the next months. As to contrib, the security team could still do periodic checks grepping for wrong usages; people can still use other bad practices such as t($user_input), t('<a href="@user_input">link</a>', ['@user_input' => $url]) and eval() there as well so it's not like this is something new.

To prevent site owners from using assert() on production setups (which would give a noticeable slowdown anyway, and on most security-sensitive setups with dangerous php functions turned off would be happening anyway), we could check for this in security modules such as Security Review and have a hook_requirements() that warns about the potential security issue if people leave it on on production setups (#2570951: Add Assert Active to Status Report). I discussed this with @chx and if we do this I don't think he'd oppose single quote asserts.

Granted this will all cease to be an issue in PHP7 but over the next years we're stuck with supporting PHP5.

Testing Runtime Assertions

Test environments should run with assert.active on
Production environments should run with assert.active off.

So what happens if someone misuses assert() as a control structure? What then?

Complicating the issue are the tests that directly are checking runtime assertions, but since assert.active can be set at runtime this isn't a huge hurdle.

The largest concern is this - assertions are being added to help with diagnostics. If they are turned off, that's all for naught.

My proposal, if it's possible, is to set up the test runner to be able to run tests with assert.active set to off, and run just such a test whenever a patch is marked RTBC as a final check. Perhaps other production system integrity tests should be ran with assert.active set to off.

Thoughts?

The potential of breaking code through "misusing" assertions seems so rare, no matter how "rookie" the developer or how little they might understand assertions, that we ought not to be concerned on every single patch.

For this to be misused as a control structure (#8), we'd need to have an assert within a try block, and catch for an \AssertionError (which will realistically only happen in tests) or an \Exception, with code that relies on the assertion always being false and the \AssertionError being thrown. That's so unlikely it's just not something we should be doing a full extra test run for as that's just wasteful.

Maybe we could test for in CodeSniffer tests whenever we have those in CI. And maybe we could do a pre-release test of Drupal core on a "Production" testbot. Just not on every patch :)

So assertions should always be turned on on test runs for patches - as they are right now...

Another issue I've been thinking of. What is the disruption level of a failed assertion? For example, say I add an assertion to a method that asserts the first argument is an integer. A popular module out there is calling the method with a string, failing the assertion. If core is deployed with the assertion production systems will be unaffected, but development systems will fail the assertion with the module installed.

So is this 8.0.x level, or will new assertions only be added on the 8.x levels?

Keep in mind such an assertion would only be added to reflect the PHP docs, so the module in this case will have been incorrectly calling the method all along - the assertion is merely highlighting this fact.

I think it's safe to assume that since assertions are now a thing in D8, expanding their use elsewhere in core can happen basically any time as long as there's no BC break. I think that's especially true since one can disable them globally.

I wouldn't go too crazy with it until after 8.0.0 is tagged, though. In the mean time (it's only 16 days), you might be able to identify some files in core that are modified less frequently (for instance, I have a patch applied to my project for the UserData service that has applied cleanly since like beta13 or something) and start rolling patches for those files. If they haven't changed in the past few months, they probably won't change in the next 16 days (and if they do, it'll hopefully be pretty minor changes).

Changed the parent to a newer issue that makes for a more appropriate parent.

Having something that can dev break on a patch release makes me uneasy, but the only way this could become a production problem is if someone runs a production site in development mode, which shouldn't be done anyway, so it's a level of unease I can live with.

Define best practices for using and testing assertions and document them before adding assertions to core

I feel somewhat safe to close this since we now have assert() statements in core.

Feel free to re-open if we actually want to write best practices down. Actually… it looks like we've already got them: https://www.drupal.org/node/2492225