Remove REQUEST_TIME from bootstrap.php

I'm ok with the change personally but we might want to make sure we bring this to the attention of DX minded people sooner rather then later and make sure everyone is on board.

+++ b/core/tests/bootstrap.php
@@ -82,7 +82,7 @@ function drupal_phpunit_register_extension_dirs(Composer\Autoload\ClassLoader $l
-define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
+//define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);

I assume for review/testing that its only commented out?

If it's in $_SERVER, then it should be in the request object, no? We should get it from there rather than the super-global.

We could arguably stick it into the container as a parameter, too, in which case services that need it can just get that injected. We do that for the root dir now, don't we? Or is that different because it doesn't change per-request?

Because I didn't realize bootstrap.php is not bootstrap.inc.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -202,6 +202,20 @@ public function removeBin() {
+    if (defined('REQUEST_TIME')) {
+      return REQUEST_TIME;
+    }
+    else {
+      return (int) $_SERVER['REQUEST_TIME'];
+    }

could we just return (int) $_SERVER['REQUEST_TIME'];? I don't know there might be a reason not to but seems like it'd be fine to me.

beta eval to clarify

Patch didn't apply any more after #2457887: Use Utility\SafeMarkup class instead of Utility\String for placeholder(), checkPlain(),format() functions got in. Rerolled.

I was also confused about the difference between bootstrap.inc and bootstrap.php. @alexpott explained it to me. Updated issue summary.

Patch looks good, a few nitpicks.

1. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
   @@ -286,4 +286,18 @@ protected function pathAliasWhitelistRebuild($path = NULL) {
   +  /**
   +   * Wrapper method for REQUEST_TIME constant.
   +   *
   +   * @return int
   +   */
   +  protected function getRequestTime() {
   +    if (defined('REQUEST_TIME')) {
   +      return REQUEST_TIME;
   +    }
   +    else {
   +      return (int) $_SERVER['REQUEST_TIME'];
   +    }
   +  }
   

   Seeing that this method is used twice (here and in MemoryBackend), might it be useful to put this in a trait so it can be reused easily in new code that relies on REQUEST_TIME?

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -354,7 +354,7 @@ public function testTimeOptionExceeded() {
   -    $this->executable->limit = array('unit' => 'seconds', 'value' => (REQUEST_TIME - 3600));
   +    $this->executable->limit = array('unit' => 'seconds', 'value' => ((int) $_SERVER['REQUEST_TIME'] - 3600));
   

   The parentheses around the subtraction are not needed.

3. +++ b/core/tests/Drupal/Tests/Core/Cache/ChainedFastBackendTest.php
   @@ -45,7 +45,7 @@ public function testGetDoesntHitConsistentBackend() {
        // Use REQUEST_TIME because that is what we will be comparing against.
   -    $timestamp_item = (object) array('cid' => $timestamp_cid, 'data' => REQUEST_TIME - 60);
   +    $timestamp_item = (object) array('cid' => $timestamp_cid, 'data' => (int) $_SERVER['REQUEST_TIME'] - 60);
   

   The line of documentation right above this still refers to the REQUEST_TIME constant.

4. +++ b/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php
   @@ -207,7 +207,7 @@ public function testGetAliasByPathNoMatch() {
   -      ->with($this->cacheKey, array($language->getId() => array($path)), REQUEST_TIME + (60 * 60 * 24));
   +      ->with($this->cacheKey, array($language->getId() => array($path)), (int) $_SERVER['REQUEST_TIME'] + (60 * 60 * 24));
   

   These parentheses around the product are also not strictly needed, but this is OK as it helps with readability.

All unit tests are covered, we can be certain of this otherwise we would get failures with "Use of undefined constant REQUEST_TIME".

Fixed those little nitpicks. They don't change the actual functionality so I suppose I am still allowed to RTBC this.

Oops I uploaded the wrong patch when I was in a hurry to catch my train. This is the patch I was looking for.

Does this need a CR to demonstrate the proper way to get this now in test and non-test code?

This is only introduced in 8.0.x but indeed we might want to inform developers about the change. Added draft change record.

Seeing that this method is used twice (here and in MemoryBackend), might it be useful to put this in a trait so it can be reused easily in new code that relies on REQUEST_TIME?

This still isn't using a trait from what I can tell.

Sorry I forgot to mention this, @alexpott told me on IRC that he didn't think it was necessary to make a trait for that simple method.

Ah, back to RTBC then :)

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -202,6 +202,20 @@ public function removeBin() {
+  protected function getRequestTime() {
+    if (defined('REQUEST_TIME')) {
+      return REQUEST_TIME;
+    }
+    else {
+      return (int) $_SERVER['REQUEST_TIME'];
+    }
+  }
+

Do you think it is better to write this as:

return defined('REQUEST_TIME') ? REQUEST_TIME : (int) $_SERVER['REQUEST_TIME'];

It is not a blocker by any means, but I am thinking this is a quick change that saves a bunch of lines and makes the method easier to read.

I didn't realize this was a Normal issue. I avoided putting it in a patch thinking not to block a critical. But this is a small change and makes the code easier to read, so here goes...

Looks fine. I don't think it makes it easier to read; it's equally legible for me in case of such a simple function. So I think #18 & #25 are equivalent.

Back to RTBC.

I didn't realize this was a Normal issue. I avoided putting it in a patch thinking not to block a critical.

This is actually major since it blocks two major issues: #2232861: Create BrowserTestBase for web-testing on top of Mink and #2304461: KernelTestBaseTNG™.

@hussainweb
It doesn't matter here, because we don't call it often, but in case you have code which is executed a lot of times, have a look at http://fabien.potencier.org/article/48/the-php-ternary-operator-fast-or-not

@dawehner: Thanks for the links. I did come across this long back but completely forgot about it.

It was really a nitpick. It is not worth holding up a major issue for this. It's back to RTBC so there is nothing to do. I am now leaning towards #18 even though it is more verbose but I am fine with either now.

Changes like this looks great:

-    $prepared->valid = $prepared->expire == Cache::PERMANENT || $prepared->expire >= REQUEST_TIME;
+    $prepared->valid = $prepared->expire == Cache::PERMANENT || $prepared->expire >= $this->getRequestTime();

Changes like this, not so much:

-      'updated' => REQUEST_TIME,
+      'updated' => (int) $_SERVER['REQUEST_TIME'],

we've tried really, really hard over the D8 development cycle to avoid direct consultation of globals, especially superglobals.

However, in talking about this on IRC with alexpott, neclimdul, EclipseGc and Crell, it doesn't actually make the "pre-existing condition" any worse. These tests were already consulting the superglobal directly, just doing so behind a constant that we had to jankily define.

So I think this is fine for now, but we should get a follow-up to (quoting Crell) "I would really like to see if we can get the request time injectable, because then any service can just take an int as a constructor argument and all is right with the world."

Searching https://www.drupal.org/list-changes/published?keywords_description=REQUE... however I don't really see anything that covers the fact that you're not supposed to use REQUEST_TIME anymore, and the change record only covers the case we actually don't generally want people to do. It probably needs to be re-jiggered into something more like "REQUEST_TIME constant removed" and laying out the two replacement paths ("recommended" way to inject the request object, and "if you have to" way (for tests only) to consult the superglobal directly).

Once that's done, this is fine to go in.

Angie asked me to weigh in here.

Overall, this issue is reducing coupling, even if only very slightly, so I'm OK with it. We're replacing an implicit dependency on the Drupal environment with one for the PHP environment, which is a step up even if still not ideal.

It would be useful to have a follow-up for the non-test code in here to go a step further and try to inject the request time as an integer dependency from the container, which would therefore decouple those services from the RequestStack. Problem is, the container doesn't support non-object services or runtime-generated parameters. However... it does support runtime object services, so we could instead of an int stick a DateTime into the container for "now" and inject that. That would have the same net decoupling result. That sounds like a good follow-up once this is committed.

I think the follow up is "as you unit test code, you can't use REQUEST_TIME anymore so things have to do something else"

So I thought I was waiting on a change record update mentioned in #30, but in grepping I see that it really is only unit tests where we no longer use REQUEST_TIME; it's still peppered throughout the code base in various places even with this patch (including KernelTestBase ones like /core/modules/system/src/Tests/Entity/EntityCrudHookTest.php).

So, sorry about the red herring.

Committed and pushed to 8.0.x. Thanks!