500 error on passing invalid month to MonthDate view argument handler

I think that in case of an invalid value, the default value should be taken from the parent class, instead of forcing an empty string, for example when a default value for "All" is provided:

I attach a patch

The basic solution look good. For both code changes I would like to improve the code a little:

+++ b/core/modules/views/src/Plugin/views/argument/MonthDate.php
@@ -24,7 +24,13 @@ class MonthDate extends Date {
+    if($timestamp){
+      $summary_name = format_date($timestamp, 'custom', $this->format, 'UTC');
+    } else {
+      $summary_name = parent::summaryName($data);
+    }
+    return $summary_name;

Can we change this to:

    if($timestamp){
      return format_date($timestamp, 'custom', $this->format, 'UTC');
    }
    return parent::summaryName($data);

Now we need automated tests so that this fix will not be undone in the future.

@daffie I updated the patch

@kkalashnikov: Thank you. Could you also create the necessary tests?

Adding some test coverage for this

Nice test coverage!

1. +++ b/core/modules/views/src/Plugin/views/argument/MonthDate.php
   @@ -24,7 +24,11 @@ class MonthDate extends Date {
   +    $timestamp = strtotime("2005" . $month . "15" . " 00:00:00 UTC");
   
   @@ -32,7 +36,11 @@ public function summaryName($data) {
   +    $timestamp = strtotime("2005" . $month . "15" . " 00:00:00 UTC");
   

   Wouldn't it be better to catch the thrown exception instead of making an assumption about what the date formatter service does?

   So just wrap the original code in a try/catch and return the parent call in the catch?

2. +++ b/core/modules/views/src/Plugin/views/argument/MonthDate.php
   @@ -24,7 +24,11 @@ class MonthDate extends Date {
   +      return \Drupal::service('date.formatter')->format($timestamp, 'custom', $this->format, 'UTC');
   
   @@ -32,7 +36,11 @@ public function summaryName($data) {
   +      return \Drupal::service('date.formatter')->format($timestamp, 'custom', $this->format, 'UTC');
   

   If we are not using a try/catch at least it seems like the date formatter is injected, so lets use that and not \Drupal:: (like in the original code)

Thanks for the review @Lendude!

I like the idea of catching the exception and returning the parent call. Uploading a patch implementing the same.

Looks great, nice work!

Committed/pushed to 9.2.x and cherry-picked to 9.1.x

This should go into 8.9.x too, but leaving at 'to be ported' for a little while since we just did the patch release yesterday evening so still thawing.

Cherry-picked to 8.9.x

The cherry-pick broke 8.9.x HEAD on all environments: https://www.drupal.org/pift-ci-job/1965195

+++ b/core/modules/views/tests/src/Functional/Plugin/MonthDatePluginTest.php
@@ -0,0 +1,101 @@
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  protected static $modules = ['node'];

ON Drupal 8.9 this needs to be public.

Here's a fix.

That'd do it.

Committed the backport to 8.9.x. Thanks!