Update REQUEST_TIME call

The idea is good, but we cannot rely on ::time() as we are so low level that we need to work within the bootstrap container as well and request() needs the request, which does not exist at that time.

Brandon already tried that.

We could rely on the timestampInvalidation() service however, which we already inject.

Main thing changes is that we effectively use time() instead of REQUEST_TIME for new entries then.

However, the ones in MemcacheStatisticsController look fine to me. I will review all handling of time today and update things to the new apis where it makes sense.

I rolled the ones in MemcacheStatisticsController into a memcache_admin commit I just did. The other bits still need further review. (committed and reverted them after I realized I had pushed the wrong part.)

We can just use our own definition of REQUEST_TIME instead once 9.x comes around.

Unless we want to change the behavior, there is no need now.

Whelp D9 is (soon) out. Re-implemented REQUEST_TIME as our own definition. People were seeing issues with the time service in the deprecated report, which I think is related to this issue. This shouldn't change the timestamps unless the global is missing. But honestly, that is what core is doing, so I don't think its much of a change.

+++ b/src/Invalidator/TimestampInvalidatorInterface.php
@@ -58,4 +58,17 @@ interface TimestampInvalidatorInterface {
+  /**
+   * Get the current request time
+   *
+   * Memcache operates within the bootstrap container, so only the REQUEST_TIME
+   * global is (possibly) available. The request object and time service aren't.
+   * Return the global or time() if need be.
+   *
+   * @see https://www.drupal.org/node/2999614 for more info.
+   *
+   * @return int
+   *   A Unix timestamp.
+   */
+  public function getRequestTime();
 }

+1 for working on this! Short review: Not sure if it covers all spots yet but it looks good and simple to me from a short glimpse. But I would maybe discuss the code comments with someone (else than me). Especially 62-75 in TimestampInvalidatorInterface.php?

REQUEST_TIME IS *not* removed in Drupal 9, that has been pushed to D10, so this is not a blocker.

Also, I think the bug around this might actually have been fixed and it might just work now, haven't tried yet with redis.

Setting postponed for after the D9 release since its no longer a blocker. Hopefully then we can refactor correctly per Berdir's comment with the time service.

REQUEST_TIME IS *not* removed in Drupal 9, that has been pushed to D10

@#11: ah! didn't know that, Thanks!

Also, I think the bug around this might actually have been fixed.

Do you have a link to an issue or some quick words helping to find the spots?

@japerry: I think we can forget #10 then ... which was made in a rush.

#3113476: Fallback when request is not available on the stack in Time service is the issue that I mentioned.

@berdir: Thanks a million!

My team uses preinstalled Drupal instance for testing and in case of a time-based caching workaround, I need to be able to mock (request) time.

Re #3

The idea is good, but we cannot rely on ::time() as we are so low level that we need to work within the bootstrap container as well and request() needs the request, which does not exist at that time.

What if we follow the same logic as we have at ::debug()? So, trying to return \Drupal::time()->getRequestTime() and if it fails, fall back to the actual logic (int) $_SERVER['REQUEST_TIME'] ?? time();

Also,

1. It is unclear to me why this issue is postponed - maybe it was just stuck in this state accidentally?
2. My suggestion is to change this to a task (or to a feature request). Imho this is not a bug anymore.

I want to share my local path with you.

* Updated (created) issue summary.
* Changed category to feature request.
* Changed priority to normal.

Feel free to change them if you disagree.

I see a test failure with the next minor core release, but imho it is a preexisting issue.

it affects performance as instead of variable (expected and documented) we'll get a set of function calls for no reason (each get doing validation and each invalidate of tag)

generally not clear why this method is public at all and moreover not clear why it has no static cache for result now

Forgot to unassign

I made two changes to the MR:

1. The static cache added in the second commit doesn't work. Once getRequestTime has captured a value it never consults the service again, so swapping datetime.time afterwards is useless. The new kernel test `MemcacheBackendTest::testRequestTimeUsesTimeService()` fails with the static cache and on 8.x-2.x, and passes with this rework.

2. On @andypost's performance point: the cost that matters is not the function call but the try/catch. `\Drupal::time()` throws when the container is not set yet, and this backend serves cache.container, so those early reads each paid for a thrown exception. The rework checks `\Drupal::hasContainer()` / `\Drupal::hasService('datetime.time')` first instead, which is a cheap method call per `valid()` on a normal request and no exception during bootstrap.