Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2

There's already some discussion of this at #2757347: Assert failure in ChainedPlaceholderStrategy.php but with a less helpful title. I thought there was another issue open about it as well, but can't find that one if there is.

This is at least major for PHP 7.2 compatibility.

I'm wondering whether we can fake around that, like by using something like one of the following options
and then switch over to the proper system, once we require php7.

\Drupal\assert(function() { return FALSE/TRUE; }, $message);

or
if (\Drupal::isDev()) { \Drupal::assert(); }

We also can't convert our assert() calls to use real PHP code because that would then be always executed on production sites using PHP 5

When we remove that we should document whether we do that due to security and or performance reasons. I would argue a bit that when you care about performance you would run with php7 anyway.

We talked a bit about just removing the string asserts as the OP suggests. I think it was Wim in irc who suggested that poses a (different) security issues in PHP 5.5/6 since asserts would run all the time, leading to the potential of XSS or sensitive data appearing in the error message.

Since we have our own assert handler though, we could remove showing messages for PHP 5/6 and only show them for PHP 7. With the performance consideration, agreed since this is neutral on PHP 7 it's not really a consideration here.

Adding an API around asserts seems tricky - since we'd then need to make contrib/custom code use it in every situation and they can just use assert() directly still without knowing about this.

@klausi I've struggled to make the following clear - runtime assertions are not just another form of exception that just happen to have the benefit of being turned off in production. They are definitions of program expectations that should be outright IMPOSSIBLE in error free code and configuration. If the condition they are testing for is possible, at all, in error free code and configuration then it is inappropriate to use them.

I think assert() calls in non test code are wrong anyway. Either you throw an exception for unexpected conditions or you just assume that the unexpected cannot happen and prove the functionality with test cases.

That only works if you and the team you're on is the only one writing code for the project. Drupal isn't limited to one team, and there are a lot of coders out there using it without writing unit tests at all. Runtime assertions define and enforce the expectations of the API. Also, you can't expect two teams that don't even know each other to write unit tests covering the use of their modules in tandem. Unit tests can only test known scenarios - runtime assertions can watch for unknown situations. They are a supplement to unit tests, not a replacement.

As to the original issue on this, switching to straight calls instead of eval strings is almost all upside except for the fact that it will make Drupal 8 run noticeably slower on 5.x than on 7.x. Then again, 5 about twice as slow as 7 anyway, so I doubt speed is the primary concern of those still using it.

My personal vote is to move to the non-eval string format and let PHP 5 users take the speed hit. I estimate that by the time runtime assertions are truly widespread in Drupal PHP 5.x will no longer be the floor.

EDIT: There's also a third option here. Many of the larger assertions pass through established function libraries. We could hotwire those to always return TRUE when assert_options('ASSERT_ACTIVE') is false. That will greatly reduce the speed hit for 5.

the problem is debugging and tracing of errors. So a developer writes assert()s in her code. She sees a strange PHP warning in the production server logs and goes looking in the code to figure out what could have caused it. She tries 20 different input variations on her dev site but everything is caught by assert() expectations, so that cannot be the problem. She tries another 20 possibilities, but it looks like that still cannot be it, because the assert()s are catching them correctly. After a long 8 hour day she has to give up and goes home. Still plagued by the problem she realizes under the shower: ugh, assert() must have been disabled on production, that's why the invalid stuff could get past it!

You are describing how to misuse assertions. I don't like to repeat myself, so I invite you to read up on what assertions are and how they are used before continuing. Here's some required reading before I'll debate you any further on this topic:

https://www.drupal.org/docs/8/api/runtime-assertions
https://api.drupal.org/api/drupal/core%21core.api.php/group/php_assert/8...
https://en.wikipedia.org/wiki/Design_by_contract

I second the idea to remove unquote assertions, as i did on https://www.drupal.org/node/2757347#comment-11510251

As Aki Tendo suggests, this lets PHP 5 users take the speed hit without affecting PHP 7 users, increasing the likelihood that PHP 5 usage on 8.x will becom insignificant faster and we might be able to break the original D8 PHP 5 compatibility promise in a not-too-far release.

Attached patch is a working proof of concept illustrating what I feel is the best approach - wrap costly assertions in logic checks to prevent them from running when assertions are turned off in PHP 5 (7 will never run them) and simple assertions just go unwrapped. If this syntax is ok I'll expand the patch to include all current uses of assert in the code base I can find (I have a list of around 100).

Test history of the patch here: https://www.drupal.org/pift-ci-job/607467

Still waiting to hear whether I should proceed with expanding the proof of concept above into a full patch.

@Aki Tendo asked me to look at this in IRC. I'm not really sure there is a framework manager decision to be made here. If something like #12 doesn't happen before PHP 7.2 there will be a patch to remove assertions from Drupal 8. If this is done before hand in a way that is simplest for developers and doesn't affect PHP7 or PHP5 performance or has minimal impact - then all is good.

I had a thought the other day on how to best approach this. This is going to get messy:

if (version_compare(PHP_VERSION, '7.0.0-dev') >= 0 || assert_options(ASSERT_ACTIVE)) {
  assert(\Drupal::service('cache_contexts_manager')->assertValidTokens($cache_contexts));
}

What if we define a constant early on? Then, because of lazy evaluation, we can dodge the performance hit with something easier to read...

assert( DRUPAL_ASSERT_OFF || \Drupal::service('cache_contexts_manager')->assertValidTokens($cache_contexts));

It's not idea, but it should both be easier to read and it should run faster. Some of the unit tests will still need to directly check version and assert mode when they are testing if assertions themselves are working correctly, but the rest of the system should be fine with this. Thoughts?

EDIT: When PHP 5 support is dropped the followup patch should be relatively easy to create - simply do a find/replace on this constant and the || operator.

Holding off on this a bit longer because of the developments in the EOL policies thread here.

https://www.drupal.org/node/2842431#comment-12201876

I also removed two lines from the issue summary but I feel I should detail why here:

We don't want Drupal 8 to throw deprecation warnings if we can avoid it. The assert() calls we have are covered with test cases anyway instead.

No, they are not, and they cannot be. Whoever wrote this betrayed a fundamental misunderstanding of what runtime assertions do and quite obviously didn't read the documentation on them. Dropping all the assertions on the cache tags will be disasterous for module developers - if during dev they introduce an invalid cache tag they'll corrupt their entire site database. Prior to assertions checking for this mistake was deemed important enough to do even in production where new module code should not be developed.

We also can't convert our assert() calls to use real PHP code because that would then be always executed on production sites using PHP 5. assert() as language construct only exists since PHP 7, see http://php.net/manual/en/function.assert.php

I see this as an inconvenience more than a problem. Use an older, slower version of the language, get an older, slower version of the site. Assertions running when they shouldn't isn't the end of the world. It's an optimization concern. And, frankly, anyone running PHP 5 isn't concerned with optimization considering how much slower it is compared to PHP 7.

If PHP 5 is being dropped within a two years I feel the time has come to just code these in the manner optimal for PHP 7 so long as PHP 5 doesn't outright break.

assert( DRUPAL_ASSERT_OFF || \Drupal::service('cache_contexts_manager')->assertValidTokens($cache_contexts));

I think this is indeed not the worst idea. We could decide to add it to just a subset of the assertions, aka. the ones which might be really costly.
Once in the future we maybe drop PHP 5 support, we then can easily change them again, but maybe keep the constant around for BC reasons.

Ok. So where should the constant be defined?

My thought is directly after the settings file is parsed we check to see if it has been defined at that point - and if it hasn't, set it to TRUE.

Can't we add the constant to \Drupal\Component\Assertion\Handle maybe?

Oh sorry I misunderstood the question, I guess right after the settings.php bit, this makes sense.

time is pressing on this issue

php7.2 release date - 30th November 2017

https://blog.martinhujer.cz/php-7-2-is-due-in-november-whats-new/

updating the issue summary

I've been wondering if we can just go ahead and accept the cost to php5 at this point. Adding additional complexity around this we will have to support doesn't seem worth optimizing for php5.

We still have to discuss Wim's concerns though because being secure is something we do have to support. I'd like to know more about those concerns.

I think what Wim means is the fact that with assertions now also running in production (on 5.6), some of these assertions may operate on live tainted data and cause security issues since anything tainted needs to be considered unsafe.

• This is not an issue on dev/staging servers where assertions usually run because even breaking these may be part of development.
• This is not an issue on 7.x production either, since assertions are turned off there, and won't be evaluated
• This is an issue on 5.6 production, because although assertions are turned off there too, the expression within them will still be evaluated

I totally agree that we should not care about the performance aspects. When you are not running yet on php 7.x, you don't take your performance that serious anyway. I think the security aspect of it though is actually problematic.

Yeah, I've been thinking a lot about that. is_null() or is_string() checks really don't matter regardless but its possible the others mean something different in the changed context.

I'd think(hope/want?) that we would have these calls be sufficiently hardened to be safe even in a development environment though since it would be easy enough for someone to leave assertions on in production by accident. Also you know development environment should be secure too.

Next question before I start on this. Shall I conform to this section of PHP 7 docs?

While assert_options() can still be used to control behaviour as described above for backward compatibility reasons, PHP 7 only code should use the two new configuration directives to control the behaviour of assert() and not call assert_options().

This means removing all calls and code within the system manipulating assert_options().

The .htaccess file can be used to manipulate zend.assertions (default 1) and assert.exception (default 0). I want the default for assert.exception to be 1 for Drupal, the 0 behavior is a PHP 5 compatibility shim. Setting it to 1 allows us to define what exception is thrown by assert (default AssertError) which makes the unit tests far easier to write and run.

What to do with zend.assertions though? If we force the value to -1 then devs will have to change it back to 1 in their sandbox and testing servers - and so will we. If we leave it alone and accept the environment default then administrators can configure their servers appropriately, but less technical users will have a performance impact. We could add something to the status report in the admincp whenever zend.assertions is set to any value but -1.

Given these questions I will deliver my patch in waves. One patch will deal only with the process of changing the assert statements themselves to the non-eval markup. A second patch will be attached to a child issue ticket i'll create after this post.

EDIT: I have created a child ticket discussing these implications. I would ask that discussion on these issues be done there leaving this ticket focused on the task of the conversion of the assert statements.

Ran across the following while working on this.

  public function setTypedDataManager(TypedDataManagerInterface $typed_data_manager) {
    assert($typed_data_manager instanceof TypedConfigManagerInterface, '$typed_data_manager should be an instance of \Drupal\Core\Config\TypedConfigManagerInterface.');

The assert statement above can never be tripped. If $typed_data_manager isn't an instance of the required class the Type hint in the function declaration will fail and PHP will throw a TypeError (or Catchable Fatal Error in PHP 5). I will remove any assertions like this I find during this pass.

EDIT - Oh, The assert is checking for a different interface. I investigated, found that TypedConfigManagerInterface extends from TypedDataInterface, so I moved that type hint to where it belongs.

EDIT 2 - attempting the above violated the constraints of the interface, so I reverted to let this particular dog sleep.

Another one, noting here:

  public function processAttachments(AttachmentsInterface $response) {
    assert('$response instanceof \Drupal\Core\Render\HtmlResponse');

HtmlResponse implements the interface that is type hinted for. The assertion makes the code more brittle than it needs to be. Asserting an additional class requirement isn't appropriate - an interface is. If the AttachmentInterface doesn't cover all the bases then an interface that does should be added and that should be type hinted for. Dropping this assert.

Ok, attached is a patch that addresses all the assert calls I know of done through a find in files search using sublime text for the string:

assert('

Conversion usually is a straight forward removal of the eval structure. However, the eval structure can't see use statements so the old assertions were forced to use the hard to read fully qualified class name. This is changed to make use of the use statement and then the class name in the assert call - it's much easier to read.

I ran into one assertion who's function was verified in a unit test. I'll be looking for others, but any unit test checking assertions need to start doing this:

if (ini_get('zend_assertions') > 0 || ini_get('assert_active') === 0) {
      $this->markTestSkipped('Assertions disabled, skipping');
    }
    else {
// the test itself is here.
}

This gives us the flexibility to setup CI systems to run the system through both scenarios of runtime assertions on and off.

Also, one assertion was replaced by a type hint. The associated unit test looking for an AssertionError had to be changed to look for a TypeError. Since these only exist in PHP 7 the test was set to skip on PHP 5.

EDIT: This patch was built against 8.4.x instead of 8.5.x as normal since this will have to be implemented ASAP because of the incoming PHP 7.2 release.

Small note here: I think our coding practice(/standard ?) now calls for the use of SomeClass::class with an imported class instead of the legacy FQCN format mentioned here like in #29. Should it not be changed when modifying these lines ?

It is being changed. The reason the fully qualified names were used at all is due to a limitation of the eval format of assert - eval strings cannot be used with use statements nor with scope operators like static or self.

Note that this patch doesn't address documentation changes to reflect this - that's part of the child.

+++ b/core/modules/big_pipe/tests/src/Unit/Render/BigPipeResponseAttachmentsProcessorTest.php
@@ -30,11 +30,16 @@ class BigPipeResponseAttachmentsProcessorTest extends UnitTestCase {
+    if (version_compare(PHP_VERSION, '7.0.0') <= 0)  {
+      $this->markTestSkipped('Type Error test skipped on less than PHP 7');
+    }

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -519,17 +519,22 @@ public function testLibraryWithLicenses() {
+    if (ini_get('zend_assertions') > 0 || ini_get('assert_active') === 0) {
+      $this->markTestSkipped('Assertions disabled, skipping');
+    }

Why do we need to make these changes? These seem to be out-of-scope to this change. Whilst they might be valid I don't think they are necessary to fix the issue at hand.

In the interest of getting this patch folded in as quickly as possible I've rolled back both these changes.

The Big Pipe snippet will be given it's own issue ticket if I remember to do so - no big loss if it's not changed though. The assertion active check for the Library Discovery Parser will appear in the child ticket as part of its changes (removing assert_options() calls from the code base and allowing unit tests to run regardless of the status of zend.assertions/assert.active

The fail involved code I hadn't touched so I re-ran the test and it came back clean.

PHP 7.2 comes out on the 30th of November. If this is to go out with the November update it needs to get moved on now, though there might not be time. If that date is missed the December update certainly should have this.

The child and grandchild patches do not need to be promoted to critical as they are more long term health of the code base issues.

I do not know enough about assertions to be able to review this patch, but I tested it in a Travis CI build that uses Drupal Console to install and display the site status.

Without the patch, the PHP 7.2 install fails, see e.g.

https://travis-ci.org/mondrake/imagemagick/jobs/293266448

with the patch, it succeeds, see

https://travis-ci.org/mondrake/imagemagick/jobs/295698097

(tests fails in both cases, but that's a different problem - just look at the drupal site:install and drupal site:status calls)

There're no PHP 7.2 testbots on DrupalCI yet, it would be good to have such and test this patch.

The goal of the patch is to avoid the deprecation errors triggered when assertions use eval strings. These deprecations where added in PHP 7.2. This patch meets the bare minimum required to get past those errors - it's children update the Drupal documentation and follow the PHP team's recommendation to cease the use of assert_options().

The patch in #34 causes the following phpcs warning:

FILE: ....dev/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
   |         |     (Drupal.Classes.UnusedUseStatement.UnusedUse)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Before applying the patch the regex \sassert\(['"] finds plenty of asserts with strings - after it none.

It'd be great to have a PHP7.2 testbot but we don't have that atm given that I've reviewed the patch with git diff --color-words and it looks great.

Considering my change is so slight (removing an unused use) I think I'm okay to rtbc this patch.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -54,7 +55,8 @@ public static function mergeContexts(array $a = [], array $b = []) {
-    assert('\Drupal\Component\Assertion\Inspector::assertAllStrings($a) && \Drupal\Component\Assertion\Inspector::assertAllStrings($b)', 'Cache tags must be valid strings');
+    assert(Inspector::assertAllStrings($a) && Inspector::assertAllStrings($b),
+      'Cache tags must be valid strings');

Supernit: Why make this two lines now? (Can probably be fixed on commit.)

I'm a little fuzzy on the 80 column rule. I presume it applies to all lines, not just comment text, so if it is possible to line break a code line to obey the 80 col rule I will, only spilling over it if the code execution requires this. I've read the coding guidelines but this is one of those fuzzy lines in it. I'll abide with whatever the community feels is best.

https://www.drupal.org/docs/develop/standards/coding-standards#linelength

I've read that pingers. :)

The very example in the docs has no line break after the && here...

if (isset($something['what']['ever']) && $something['what']['ever'] > $infinite && user_access('galaxy'))

Yet, PHP will allow a line break to either side of any boolean operator and the structure remains valid. The rule here is unclear.

No comment is made about functions with multiple lines of argument, which is the situation in the patch. I chose to line break after the first argument of assert since I was already approaching the 80 line mark. Again, there is no set rule on whether or not to use this

 someFunction($a, $b, $c);

or this

someFunction($a,
  $b,
  $c);

In practice the latter would only make sense if the argument names are much longer, not merely $a, $b and $c. My feeling is the reason there isn't a hard rule on this yet is because it's better handled case by case.

All of this is outside the scope of the patch. If the committer wants to pull the line break there I'm fine with it. I put it in out of habit and with far less thought than the discussion it's generated :)

I don't think a linebreak there is necessary. Its not a dealbreaker anyway.

There was extensive discussions above about @wimleers' security concerns up until #25, but then stopped. I asked Wim to take a look with that eye (since he only commented with a nitpick most recently :).

Also this would need to go to 8.5.x first.

The security concerns are unchanged, and unaddressed.

The chance is very slim that there is a security problem. But the fact remains that there is a certain new security risk being opened up here. PHP 7.2 is forcing our hand here though, so I'm not sure how much we can do about it. That's also why I haven't brought it up again.

Here's a patch which fixes the line break issue - the line has got shorter in the patch - so why break it in the patch :)

@Gábor Hojtsy you are right to bring up @Wim Leers's security concerns again. I think any rtbc comment should address them and mine did not so un-rtbcing.

I looked through all the asserts. I think we have to think calling is_null() / is_string() etc... on the variables even if they contain user generated content is safe. As far as I can see we need to pay careful attention to:

• \Drupal\Core\Asset\LibraryDiscoveryParser::validateCssLibrary
• \Drupal\Core\Cache\Context\CacheContextsManager::assertValidTokens()
• \Drupal\Component\Assertion\Inspector

Looking at each in turn:

LibraryDiscoveryParser::validateCssLibrary

  public static function validateCssLibrary($library) {
    $categories = [];
    // Verify options first and return early if invalid.
    foreach ($library as $category => $files) {
      if (!is_array($files)) {
        return 2;
      }
      $categories[] = $category;
      foreach ($files as $source => $options) {
        if (!is_array($options)) {
          return 1;
        }
      }
    }

    return 0;
  }

This is only doing an is_array() - I think this is safe.

CacheContextsManager::assertValidTokens()

  public function assertValidTokens($context_tokens) {
    if (!is_array($context_tokens)) {
      return FALSE;
    }

    try {
      $this->validateTokens($context_tokens);
    }
    catch (\LogicException $e) {
      return FALSE;
    }

    return TRUE;
  }

  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));
      }
    }
  }

I think we are safe here too. The checking in \Drupal\Core\Cache\Context\CacheContextsManager::validateTokens() seems all about if something is in an array and the exception is eaten and converted to a Boolean.

\Drupal\Component\Assertion\Inspector

All the methods in here do safe operations as far as I can see.

The other thing we need to be careful of is what is in the messages.

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -451,9 +451,9 @@ public static function escape($text) {
+    assert(empty(array_diff(array_keys(parse_url($scheme_and_host)), ["scheme", "host", "port"])), '$scheme_and_host contains scheme, host and port at most.');
+    assert(isset(parse_url($scheme_and_host)["scheme"]), '$scheme_and_host is absolute and hence has a scheme.');
+    assert(isset(parse_url($scheme_and_host)["host"]), '$base_url is absolute and hence has a host.');

We are printing variables in the messages. Looking at \Drupal\Component\Assertion\Handle::register() it shows that we are converting the assertion error to an exception and we know that error messages are escaped - otherwise an exception or PHP error might be unsafe for Drupal. All the escaping is handled in _drupal_log_error() - see the use of SafeMarkup::format() in that function.

So I'm giving a tentative +1 to rtbc'ing the patch - but given that this is a security concern I think we should have more than one person say that.

tagging appropriately.

I'm honestly not concerned about CODE in assert(CODE) calls in core. I'm concerned about those in contrib and custom modules.

But:

PHP 7.2 is forcing our hand here though, so I'm not sure how much we can do about it.

So … it's a language feature that became less safe. There's nothing we can do about that I think.

It was security that motivated the PHP 7.2 change though. For optimal performance PHP 5 requires assertions to be constructed as eval strings. We all know the dangers of eval, but considered it worth the risk when assertions were added. Now they must be in normal code mode.

The largest danger in assert is someone mistakenly treating it as a control structure, causing code to fail when the system is put into production mode. This is especially true in PHP 7 where assertions can be stepped around entirely - their code doesn't even get compiled - so for properly configured production systems assertions don't present a danger.

The child ticket to this goes the next step the PHP group recommends and removes assert_options() from the mix. This change makes it incumbent on the admins to set their configurations up correctly for maximum performance. Once that issue is added we'll have the option of running continuous integration tests with assertions on or off as desired.

It is btw mostly ready. It has two unwanted assert_option(ASSERT_EXCEPTION) calls needed to clear the test runners in their current configuration. I've put in an issue ticket to have that setting change that is still in the ether.

Also just fixed this in the JSON API contrib module: #2920892: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2.

And doing the same in the CDN contrib module: #2920900: Remove all assert('string') calls from CDN because deprecated in PHP 7.2.. Also tagged both PHP 7.2.

PHP 7.2 has been released today and does include that deprecation http://php.net/manual/en/migration72.deprecated.php

I built a php 7.2 container today for drupalci, and the preliminary results are pretty mangled.

https://dispatcher.drupalci.org/job/drupalci_test_containers/837/console...

So I've signed off on the security concerns in #51 but asked for more than one person too which @Wim Leers kinda of did in #53 but didn't rtbc. It would be great if someone could review this. As @Wim Leers says I'm not sure what else we can do here.

+++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
@@ -426,7 +426,7 @@ protected function _shift_boundaries($lines, &$changed, $other_changed) {
-          $this::USE_ASSERTS && assert('$j < $other_len && ! $other_changed[$j]');
+          $this::USE_ASSERTS && assert($j < $other_len && ! $other_changed[$j]);

The ! here doesn't seem right.

Otherwise this all seems great, I agree with Wim, there is nothing we can do because the language has changed.

Well, there *is* one thing we can do: Remove them.

I'm not saying we should, I'm not sure. But I know that some of those asserts are a serious performance issue, for example those in the Cache class because that is called *a lot*.

If we would remove them, we could have a follow-up of the PHP 5 deprecation issue to add them back in a year or so.

@borrison_ I agree, but changing the underlying logic of the code is outside the scope of this ticket. I just removed the quotes on this pass.

@alex_pott While this can go out the door alone, it would be better with it's child. For one, that child contains the changes in the documentation on how we handle assertions. The child is ready to go except for a change to the Drupal CI testers to set assert_exception to 1 so that I can remove two lines from the patch that are turning on asserts as exceptions.

@Berdir The needs of the many outweigh the needs of the few. PHP 5 users are the few now. A bad cache key can make a dev environment unusable and incur a massive headache to a dev that makes the mistake. It isn't fair to ask the PHP 7 devs to have to deal with that so that the PHP 5 devs can have the optimal performance for their platform, especially since their platform runs twice as slow as PHP 7 anyway - signifying that speed isn't their primary concern.

I sympathize with the boat they are in, but I feel our obligation at this point is to make sure the code works on PHP 5, not necessarily works optimally on PHP 5 since PHP 5 isn't an optimal set up to begin with.

@effulgentsia, @xjm, @catch, @plach and I discussed this on a triage call.

We agreed to keep it as critical because it impacts our ability to run on supported versions of PHP

@Mixologic is there a chance we could test 7.2 with this patch?

The consensus here is that php 5 has to bear the cost of the runtime execution, I don't think we have a choice. But it would be good to see what sort of performance regression it creates, it would also add weight to #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 if it was significant, so tagging for profiling - but that won't hold up the patch.

In the interest of making sure this fact doesn't fly under the radar, it should be noted that the child ticket also removes assert_options() calls to turn assertions off in PHP 7 at runtime following the PHP Group's recommendation. This means that in the default PHP configuration assertions will always run even on PHP 7. Most hosting providers turn them off, but people who've set up their servers themselves could see a performance loss. It's something worth monitoring, but I don't think it's breaking. Also, the option of intentionally running all tests with assertions turned off will add some of peace of mind worth the inconvenience of having to switch off assertions outside of Drupal for optimum performance.

This might be silly, but how about just marking 8.4/8.5 as incompatible with PHP >= 7.2 ? By the time we reach 8.6, many more PHP 5.x sites will have switched and supporting them will be a much less significant need. Also 7.2 is pretty minor feature- or performance-wise when compared with 7.0 or even 7.1, so there is not much to gain deploying it anyway.

That's never been done before. It could set a nasty precedent. Worst, it's the kind of decision Wordpress fanbois would latch onto when attacking the Drupal community.

Looking at the test failure of #48 under PHP 7.2, we have now DrupalCI evidence of something I reported earlier in #2885309-46: [PHP 7.2] each() function is deprecated.

While #2885309: [PHP 7.2] each() function is deprecated removed calls to each() in Drupal's codebase, PHP Unit tests still fail on PHP 7.2, apparently because PHP Unit itself has calls to each() in the release we currently use, 4.8.36, and they are now reported as deprecations.

See https://github.com/sebastianbergmann/phpunit/blob/4.8/src/Util/Getopt.ph...

The PHPUnit 5.7 branch has the calls to each() prefixed with the error suppression operator @, see https://github.com/sebastianbergmann/phpunit/commit/20c70e0b72eb92367d00...

Unfortunately, there will be no more releases for the 4.8 series, https://github.com/sebastianbergmann/phpunit/pull/2773

Example:

Batch.Drupal\KernelTests\Core\Batch\BatchKernelTest
✗	
Drupal\KernelTests\Core\Batch\BatchKernelTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-94.xml:
PHPunit Test failed to complete; Error: PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Batch\BatchKernelTest
.

Time: 615 ms, Memory: 4.00MB

OK (1 test, 6 assertions)

Other deprecation notices (1)

The each() function is deprecated. This message will be suppressed on further calls: 1x

@mondrake the each() needs to be discussed in a separate issue - I created #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2

@alexpott yes sure that's a separate issue, sorry for going OT

Unrelated random fail - back to rtbc

This still has the "Needs profiling" tag added by @larowlan in #65. I think that forward compatibility for 7.2 is more important than accommodating EOL-bound PHP 5 versions, but it would be good to know for sure whether there's a regression and what it's like.

Saving issue credit for people who participated in the discussion. I'm also going to queue a test run with the new 7.2 environment.

Just to say I'm personally not concerned about the profiling here - it's always nice to know, but we know there's no impact on PHP7+ and we don't really have a choice here, so the trade-offs are already understood. Where we normally need profiling is on the issues where no-one thinks about profiling the change and we find out months after they've gone in what the impact was.

> but we know there's no impact on PHP7+

*if* properly configured, I guess the major hosting platforms by now properly use the recommended production settings now but I know it took some of them quite some time to configure it like that by default.

Triggered a re-run of tests under PHP 7.2 - the count() issue should now go away after commit of #2915820: [PHP 7.2] FormValidator: Parameter must be an array or an object that implements Countable (tests will fail anyway, but just to see)

Just to say I'm personally not concerned about the profiling here - it's always nice to know, but we know there's no impact on PHP7+ and we don't really have a choice here, so the trade-offs are already understood.

+1

*if* properly configured, I guess the major hosting platforms by now properly use the recommended production settings now but I know it took some of them quite some time to configure it like that by default.

True, but they've had plenty of time now to fix that. Drupal 8 has been out for >2 years. I think that's enough time for them to fix their setup & processes. (I say this as an employee of Acquia, and I know our hosting definitely took "quite some time" to configure this correctly.)
If there's now extra reasons, extra pressure for hosting companies to finally get this right, then great.

(I'd say the same if Acquia was still lagging in this area. We need to do what's right for Drupal.)

*if* properly configured,

To be clear, this issue ticket in and of itself doesn't require a config change to remain optimal. It's the child ticket, which removes calls to assert_option(), that will require config changes for optimum performance.

A random fail in Drupal\Tests\views_ui\Functional\ExposedFormUITest for 404 - weird.

#2924753: Random fail in ExposedFormUITest

Here's some profiling.

Scenario:

• Standard install profile
• Viewing admin/content with cold cache
• 1 article node

For 5.6, 257 calls to assert took 4.9ms

For 7.0, no calls to assert - to be expected (zend.assertions is -1)

Net difference is 4% for time, 54% for RAM. Both of those within the thresholds of php 5.6 to php 7 differences anyway.

5.6 trace https://blackfire.io/profiles/f8202c54-fa52-455f-82d6-ec24209dfe1f/graph
7.0 trace https://blackfire.io/profiles/c5075b75-10d6-4060-8bde-e250429212a1/graph

Removing the needs profiling tag, not that we can do anything about the cost on php 5.6, but at least we have some measure.

Thanks for all the work on this everyone.

Committed/pushed to 8.5.x, thanks!