ResourceResponseValidator::validateResponse() is executed on PHP 7, even when assertions aren't active [#3040451]

According to #2920892-13: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2, there was a desire to only run ResourceResponseValidator::validateResponse() when assertions are active. In other words, not on production.

However, ResourceResponseValidator::doValidateResponse() has the following code:

    if ( PHP_MAJOR_VERSION >= 7 || assert_options(ASSERT_ACTIVE)) {
      assert($this->validateResponse($response, $request), 'A JSON:API response failed validation (see the logs for details). Please report this in the issue queue on drupal.org');
    }

That means that for PHP 7, the first part of the if statement is true, so then we enter the assert() call, and since the first argument is an expression (not a string), it's evaluated regardless of whether assertions are active.

Any reason not to just remove the PHP version check?

Comments

Comment #1

15 March 2019 at 04:39

effulgentsia created an issue. See original summary.

Comment #2

gabesullice

English

CreditAttribution: gabesullice at Acquia commented 15 March 2019 at 14:34

In PHP 7, assertions are a language construct and have zero cost even if the first argument is an expression.

This brings up a good point though, we should remove the wrapping if statement when PHP 5 support ends.

Comment #3

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 15 March 2019 at 14:38

Status:	Active	» Postponed (maintainer needs more info)
Issue tags:		+API-First Initiative
Related issues:		+#2917260: Validate against specific JSON Schemas if Schemata is present

#2917260: Validate against specific JSON Schemas if Schemata is present introduced that if-test, specifically the #2917260-40: Validate against specific JSON Schemas if Schemata is present interdiff. AFAICT @e0ipso in #2917260-37: Validate against specific JSON Schemas if Schemata is present asked for the validation to only happen in PHP 7 since assert() runs always on PHP 5.

That's why until the release of PHP 7.2, the parameter to assert() was in strings, to ensure PHP 5 wouldn't evaluate it, and hence have no performance impact. But #2853503: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2 was forced to change that due to PHP 7.2's BC break.

But this:

That means that for PHP 7, the first part of the if statement is true, so then we enter the assert() call, and since the first argument is an expression (not a string), it's evaluated regardless of whether assertions are active.

is inaccurate, PHP 7 literally ignores assert() lines in the codebase. That's why we did #2454649: Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts) for example.

So this if-test is saying: "if I'm on PHP 7, always enter this if-test since PHP 7 will only actually execute any code in here if asserts are actually enabled" and "if I'm not on PHP 7, then if assertions are explicitly enabled on PHP 5, then enter this if-test, because if I don't check that manually first, PHP 5 will happily execute everything in an assert() call".

Comment #4

effulgentsia CreditAttribution: effulgentsia at Acquia commented 15 March 2019 at 17:44

Status:

Postponed (maintainer needs more info)

» Closed (works as designed)

I see. I was running locally with a php.ini that didn't include zend.assertions, so it was running with the default of 1 (development mode). Setting it to 0 either in the .ini file or at the beginning of index.php does in fact make the assert() not evaluate the assertion expression.

However, Drupal's .htaccess sets assert.active to 0, but not zend.assertions to 0, so that means we can still be in a situation where people are running with the default of 1 on production. I'll see if I can find a core issue about that.