Remove views.module BC layers

Started from original patch and ran drupal-check. Fixed all deprecation warnings (usage of REQUEST_TIME constant) and created a new patch.

Don't really get why the cacheable attribute needs to be removed. If that is required it should be clear why this needs to be done. Leaving this on 'Needs work' because obviously I am missing something.

Thanks for getting this back on track @Stefdewa, this looks very close! I found only two problems in the patch:

1. +++ b/core/modules/views/src/Plugin/views/argument/Date.php
   @@ -107,7 +107,7 @@ public function defaultArgumentForm(&$form, FormStateInterface $form_state) {
   -      return date($this->argFormat, REQUEST_TIME);
   +      return date($this->argFormat, \Drupal::time()->getRequestTime());
   
   +++ b/core/modules/views/src/Plugin/views/cache/Time.php
   @@ -156,7 +156,7 @@ protected function getLifespan($type) {
   -      $cutoff = REQUEST_TIME - $lifespan;
   +      $cutoff = \Drupal::time()->getRequestTime() - $lifespan;
   
   +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -85,7 +85,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      $date_formats[$machine_name] = $this->t('@name format: @date', ['@name' => $value->label(), '@date' => $this->dateFormatter->format(REQUEST_TIME, $machine_name)]);
   +      $date_formats[$machine_name] = $this->t('@name format: @date', ['@name' => $value->label(), '@date' => $this->dateFormatter->format(\Drupal::time()->getRequestTime(), $machine_name)]);
   
   @@ -145,7 +145,7 @@ public function render(ResultRow $values) {
   -      $time_diff = REQUEST_TIME - $value;
   +      $time_diff = \Drupal::time()->getRequestTime() - $value;
   
   +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1686,7 +1686,7 @@ public function preExecute($args = []) {
   -    $this->dom_id = !empty($this->dom_id) ? $this->dom_id : hash('sha256', $this->storage->id() . REQUEST_TIME . mt_rand());
   +    $this->dom_id = !empty($this->dom_id) ? $this->dom_id : hash('sha256', $this->storage->id() . \Drupal::time()->getRequestTime() . mt_rand());
   

   AFAICT we should inject the datetime.time service here.

   OTOH … that itself ironically introduces some BC break risk 🤦‍♂️

   So … I think we should keep this as-is in the patch, but add a // @todo … comments to all of the occurrences I quoted (which excludes all occurrences in tests, where using \Drupal::… is fine), with those comments pointing to a follow-up issue we already create here.

2. +++ b/core/modules/views/src/ViewsData.php
   @@ -131,22 +131,17 @@ public function getAll() {
   -  public function get($key = NULL) {
   -    if (!$key) {
   -      @trigger_error('Calling get() without the $key argument is deprecated in drupal:8.2.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/3090442', E_USER_DEPRECATED);
   -      return $this->getAll();
   -    }
   +  public function get($key) {
        if (!isset($this->storage[$key])) {
   

   Hm … 🤔

   Calls that do ->get() will now result in a !isset($this->storage[NULL]) check getting evaluated, which does not really make sense. I think this is intended to throw a \InvalidArgumentException when $key === NULL, with the message saying ::getAll() should be called instead.

P.S.: thanks to this patch I also spotted #3097453-15: Remove system.module BC layers 🥳🙏

Just like #2893804: Remove rest.module BC layers, we should IMHO not mix removing BC layers with removing deprecated calls like REQUEST_TIME or that new symfony event stuff. That's making this harder to review and it's not what this issue is about. REQUEST_TIME has separate issue and AFAIK the event class renames do too.

Woops, sorry for being overzealous :)

I removed the REQUEST_TIME changes and updated the get function following to the feedback in #10.

No worries 😊

Here's the interdiff for #9 → #12.

#12 did address both points in #10. The first point was addressed in a different way per @Berdir's remark in #11 (and removing the needs followup tag). For the second point, this change can be found in the interdiff for #12:

+++ b/core/modules/views/src/ViewsData.php
@@ -142,6 +142,9 @@ public function getAll() {
+    if ($key === NULL) {
+      throw new \InvalidArgumentException('A valid cache entry key is required. Use getAll() to get all table data.');
+    }

👍

There are a few coding standards violations in here stillDrupalCI will fail coding standards — oddly enough @andypost already fixed those in #5. I suspect you started from #2 instead of #5, @Stefdewa? Anyway, no big deal obviously, fixed that for ya :)

And with that, this is RTBC! 🚢

Thanks! 🥳

+++ b/core/modules/views/src/ViewsData.php
@@ -131,21 +131,19 @@ public function getAll() {
-  public function get($key = NULL) {
-    if (!$key) {
-      @trigger_error('Calling get() without the $key argument is deprecated in drupal:8.2.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/3090442', E_USER_DEPRECATED);
-      return $this->getAll();
+  public function get($key) {
+    if ($key === NULL) {
+      throw new \InvalidArgumentException('A valid cache entry key is required. Use getAll() to get all table data.');
     }

This is a subtle behaviour change. Before if you passed in an empty string you'd get all data back and you would not end up setting up a cache entry for an empty string. Also we should should the legacy test to test whatever we decide to do here.

+++ b/core/modules/views/src/ViewsData.php
@@ -131,21 +131,19 @@ public function getAll() {
    * Gets data for a particular table, or all tables.

This comment needs updating as "all tables" is now incorrect.

Before if you passed in an empty string you'd get all data back and you would not end up setting up a cache entry for an empty string.

… but … that old behavior is exactly what the deprecation was about?

Oh, no, the deprecation was about $key = NULL, not about $key = ''. Good catch!

Addressed #15 and #16, also removed some @see tags that refer to deprecations that have now been removed.

1. +++ b/core/modules/views/src/ViewsData.php
   @@ -129,20 +127,16 @@ public function getAll() {
   -   * @see getAll()
   

   I think this one was fine actually, but don't feel strongly about this.

2. +++ b/core/modules/views/tests/src/Unit/ViewsDataTest.php
   @@ -637,4 +637,16 @@ public function testCacheCallsWithoutWarmCacheAndGetMultipleTables() {
   +  /**
   +   * Tests that getting data with an empty key throws an exception.
   +   *
   +   * @covers ::get
   +   */
   +  public function testGetEmptyKey() {
   +    $this->expectException(\InvalidArgumentException::class);
   +    $this->expectExceptionMessage('A valid cache entry key is required. Use getAll() to get all table data.');
   +
   +    $this->viewsData->get('');
   +  }
   

   Ideally this would use a @dataProvider to once test with '' and once with NULL?

I didn't think the reference to getAll() added anything. Added a dataProvider and also tested the edge case of 0 which is also "empty".

RTBC++

Committed 18d5319 and pushed to 9.0.x. Thanks!