#1893906: Move views argument date handlers from node module changed the date argument handlers from node_created_* to date_* however, when these got changed the created_year_month got changed to date_year_month, which is wrong. This needs to stay as created (like all the others) but use the date_year_month plugin.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olli’s picture

Makes sense. Test?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work
Issue tags: +Needs tests
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.01 KB
2.05 KB

Here's a test.

damiankloip’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/ProvidedViewsTest.phpundefined
@@ -0,0 +1,68 @@
+class ProvidedViewsTest extends ViewTestBase {

This seems a bit confusing with the DefaultViewsTest(s) couldn't testArchiveView just move into Drupal\views\Tests\DefaultViewsTest?

+++ b/core/modules/views/lib/Drupal/views/Tests/ProvidedViewsTest.phpundefined
@@ -0,0 +1,68 @@
+    $column_map = array('nid' => 'nid', 'created_year_month' => 'created_year_month', 'num_records' => 'num_records');

I don't think we do anywhere else, but maybe drupal_map_assoc() instead? Not sure if there is any point though - Just would make it slightly easier to read imo.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
dawehner’s picture

FileSize
1.98 KB
2.93 KB

Let's do that.

damiankloip’s picture

This looks good now. Let's wait for the bot.

olli’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll

curl https://drupal.org/files/drupal-1960888-6.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3003  100  3003    0     0  33492      0 --:--:-- --:--:-- --:--:-- 39000
error: patch failed: core/modules/node/node.views.inc:270
error: core/modules/node/node.views.inc: patch does not apply
jibran’s picture

FileSize
2.95 KB

Fixed this

++<<<<<<< HEAD
 +  $data['node_field_data']['date_year_month'] = array(
++=======
+   $data['node']['created_year_month'] = array(
++>>>>>>> 6

with this

-   $data['node_field_data']['date_year_month'] = array(
 -  $data['node']['created_year_month'] = array(
++  $data['node_field_data']['created_year_month'] = array(
jibran’s picture

Status: Needs work » Needs review
klonos’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

+1

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

alexpott’s picture

Status: Fixed » Needs work

This unfortunately broke head...

Fail      Other      DefaultViewsTest.  209 Drupal\views\Tests\DefaultViewsTest
    Identical result set.

Reverted with commit 6801679 and pushed to 8.x.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -VDC

#10: 1960888-10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, 1960888-10.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
858 bytes
3.09 KB

Fixed the test.

jibran’s picture

FileSize
755 bytes
3.09 KB

Full stop missing. :/

damiankloip’s picture

This change looks good. let's get confirmation from the bot. Then I think this can be RTBC again.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Came back green, thanks for fixing that @jibran!

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
582 bytes
2.97 KB

Oh wow ... this code would have passed in the last month but then started to fail.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Ignore my patch as it is pretty much the same fix, not even better.

olli’s picture

Status: Reviewed & tested by the community » Needs work

Re #19

+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.php
@@ -160,4 +160,55 @@ function createTerm($vocabulary) {
+    // Create time of additional nodes created in the setup method.
+    $created_year_month = date('Yd', time() - 3600);

This should be 'Ym', but in general, why use time() instead of a fixed timestamp also in the setup method?

jibran’s picture

Status: Needs work » Needs review
FileSize
728 bytes
3.09 KB
201.33 KB

Fixed #24.

jibran’s picture

FileSize
3.09 KB

Ignore last patch wrong rebase.

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined
@@ -160,4 +160,55 @@ function createTerm($vocabulary) {
+    $created_year_month = date('Ym', time() - 3600);

So yeah let's use REQUEST_TIME for consistency as it also fixes some random fails.

jibran’s picture

FileSize
1021 bytes
3.37 KB

Let's do it.

olli’s picture

Status: Needs review » Reviewed & tested by the community

That's a good idea, and great work jibran!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed aba632c and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.