Warning: date_timezone_set() expects parameter 1 to be DateTime

[AlexShapka]

Enabling this module and going to admin/structure/demo gives:

Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (line 2377 of /var/www/docroot/core/includes/common.inc).
Warning: date_format() expects parameter 1 to be DateTimeInterface, boolean given in format_date() (line 2399 of /var/www/docroot/core/includes/common.inc).

[AlexShapka]

Here is the backtrace:

(Array, 20 elements)
19: date_timezone_set() (Array, 1 element)
18: format_date() (Array, 2 elements)
17: demo_manage_form() (Array, 2 elements)
16: call_user_func_array() (Array, 1 element)
15: backdrop_retrieve_form() (Array, 2 elements)
14: backdrop_build_form() (Array, 2 elements)
13: backdrop_get_form() (Array, 2 elements)
12: call_user_func_array() (Array, 1 element)
11: system_block_view() (Array, 2 elements)
10: call_user_func_array() (Array, 1 element)
 9: module_invoke() (Array, 2 elements)
 8: BlockLegacy->buildBlock() (Array, 2 elements)
 7: BlockLegacy->getContent() (Array, 2 elements)
 6: LayoutRendererStandard->renderBlock() (Array, 2 elements)
 5: LayoutRendererStandard->renderBlocks() (Array, 2 elements)
 4: LayoutRendererStandard->renderLayout() (Array, 2 elements)
 3: LayoutRendererStandard->render() (Array, 2 elements)
 2: layout_route_handler() (Array, 2 elements)
 1: menu_execute_active_handler() (Array, 2 elements)
 0: main() (Array, 2 elements)
file (String, 12 characters ) index.php:21
args (Array, 1 element)
q (String, 20 characters ) admin/structure/demo

[AlexShapka]

I've traced the error to the line:

$reset_date = config_get('demo.settings', 'demo_reset_last');

in the demo.admin.inc file, which always returns the 'novalue' string if no snapshot was reset before. That in turn breaks the markup condition in the following code block:

$form['status']['reset_last'] = array(
      '#type' => 'item',
      '#title' => t('Last reset'),
      '#markup' => $reset_date ? format_date($reset_date) : t('Never'),
    );

[AlexShapka]

So changing the #markup line to:

'#markup' => is_numeric($reset_date) ? format_date($reset_date) : t('Never'),

takes care of the error.

[klonos]

Thanks for working on this @AlexShapka 👍 ...is demo_reset_last saved as a timestamp in config? If so, then perhaps we should be checking for something more future-proof than is_numeric(). I have found this solution from https://stackoverflow.com/questions/2524680/check-whether-the-string-is-a-unix-timestamp to be interesting:

I came across the same question and created the following solution for my self, where I don't have to mess with regular expressions or messy if-clauses:

/**
 * @param string $string
 * @return bool
 */
public function isTimestamp($string)
{
    try {
        new DateTime('@' . $string);
    } catch(Exception $e) {
        return false;
    }
    return true;
}

I'm even keen on adding this as a helper function in Backdrop core for better DX (so that we can reuse it in other contrib and in core).

[klonos]

...perhaps we can even do something like $string = (string) $string to ensure that this works even in the cases where the timestamp has been saved as numeric/integer.

[AlexShapka]

Thanks for working on this @AlexShapka 👍 ...is demo_reset_last saved as a timestamp in config?

I believe it is always timestamp, however the problem here is that when there was no any reset yet, then the line #61 of the file:

$reset_date = config_get('demo.settings', 'demo_reset_last');

returns the novalue string.

I am not sure what is the ideal solution here, but I thought that since the $reset_date variable is always supposed to be an integer, simple condition check if it is indeed an integer would suffice.

[klonos]

Here's what I think we should do:

  $reset_date = config_get('demo.settings', 'demo_reset_last');

  // @todo: change this check to use is_valid_timestamp().
  // See https://github.com/backdrop/backdrop-issues/issues/4132
  try {
    // Prepending '@' as the $time parameter denotes UNIX timestamp.
    $reset_date = new DateTime('@' . $reset_date);
  }
  catch(Exception $e) {
    $reset_date = FALSE;
  }

  $form['status']['reset_last'] = array(
    '#type' => 'item',
    '#title' => t('Last reset'),
    '#markup' => $reset_date ? format_date($reset_date) : t('Never'),
  );

Once we have a proper is_valid_timestamp() helper function in core (or whatever we end up naming the function), we should change the above code to this:

  $reset_date = config_get('demo.settings', 'demo_reset_last');

  $form['status']['reset_last'] = array(
    '#type' => 'item',
    '#title' => t('Last reset'),
    '#markup' => is_valid_timestamp($reset_date) ? format_date($reset_date) : t('Never'),
  );

Then we should create a new release of the module, and specify dependencies[] = system (<1.15) (assuming that the new helper function is introduced in Backdrop 1.15.0).

[AlexShapka]

How about:

$reset_date = config_get('demo.settings', 'demo_reset_last');
  // @todo: change this check to use is_valid_timestamp().
  // See https://github.com/backdrop/backdrop-issues/issues/4132
  try {
    // Prepending '@' as the $time parameter denotes UNIX timestamp.
    $reset_date = new DateTime('@' . $reset_date);
  }
  catch(Exception $e) {
    $message = t("<strong>Error:</strong> @message <br>", array('@message' => $e->getMessage()));
    $message .= t("<strong>File:</strong> @file <br>", array('@file' => $e->getFile()));
    $message .= t("<strong>Line:</strong> @line", array('@line' => $e->getLine()));
    watchdog('PHP', $message, array(), $e->severity);
    $reset_date = FALSE;
  }

?

This way it gives nice and informative message like so:

[AlexShapka]

@klonos I believe introducing the function for catching exceptions to core as you suggested could be really useful tool for easier troubleshooting the errors. However, for this particular case we might want to retreat and come up with the different solution.

The API page for the config_get() function on https://api.backdropcms.org/api/backdrop/core%21includes%21config.inc/function/config_get/1 states:

Return value
mixed The contents of the requested config option. Returns NULL if the specified option was not found in the file at all.

So provided no any snapshot executed yet, it was supposed to return NULL, however we always have the novalue string as returned value, because for some reason it is explicitly set in the demo.settings.json file:

{
    "_config_name": "demo.settings",
    "demo_dump_path": "demo",
    "demo_reset_last": "novalue",
    "demo_reset_interval": "novalue",
    "demo_dump_cron": "demo_site"
}

and also in the demo.install file:

/**
 * Implements hook_update_N().
 */
function demo_update_1000() {
  $config = config('demo.settings');
  $config->set('demo_dump_path', update_variable_get('demo_dump_path', 'demo'));
  $config->set('demo_reset_last', update_variable_get('demo_reset_last', 'novalue'));
  $config->set('demo_reset_interval', update_variable_get('demo_reset_interval', 'novalue'));
  $config->set('demo_dump_cron', update_variable_get('demo_dump_cron', 'demo_site'));
  update_variable_del('demo_dump_path');
  update_variable_del('demo_reset_last');
  update_variable_del('demo_reset_interval');
  update_variable_del('demo_dump_cron');
}

So maybe the right way of fixing this issue, would be not setting any value for demo_reset_last at all or setting it as an empty string, and then we won't have to meddle with the currently existing code at all, because in that case the line '#markup' => $reset_date ? format_date($reset_date) : t('Never'), would correctly set to t('Never') as $reset_date returns empty.

What do you think?

[AlexShapka]

New PR with clean solution created on #4

[klonos]

Thanks for working on this @AlexShapka 👍 ...I have merged this change, but please see my comment in the PR re update hooks (for future reference).

[klonos]

...I still think that we should address the core issue in backdrop/backdrop-issues#4132 in a way that benefits core and contrib.