Remove the try/catch block from DateTimeComputed::getValue()

I want to consider it some more, but I think that removing the try/catch block (as in #23) is the right thing to do.

I do not see any discussion here of when/why the try/catch block was added, so I did a little Git research. It was added in #1844956: Optimize date formatting performance → without any discussion. It is a little hard to find, since the code was moved in #2083785: Add support for determining which field properties are cacheable → .

Unfortunately, I have to set the status back to NW for a really minor point. When we remove the try/catch block and reindent the code, we have to re-wrap the comments to 80 characters. That is a novice task.

I am working on point which is mentioned in #34.

I have re-wrap the comments to 80 characters. Please review.

@sourabhjain: Thanks for updating the patch. Next time, please remember to remove the Novice tag and "cross off" the task from the issue summary. I am doing that now.

The patch in #36 also breaks the long line (originally the first line of the try block). +1 for that and +1 for RTBC.

If I have a chance later, I will rewrite the issue summary. I would like to save the next reviewer from having to read all the comments.

I think this issue is more of a task than a bug report, so I am changing the category.

I am updating the issue summary, using the standard headings.

Probably the try/catch was added in #1844956: Optimize date formatting performance → in order to get tests to pass. If anyone is curious, they can look through the failing tests and the interdiffs on that issue to figure out exactly which test was failing. Probably the test for NULL added in #3240181: \Drupal\datetime\DateTimeComputed::getValue() causes deprecations on PHP 8.1 → was the right fix all along.

The tests in Drupal core all pass, for either the patch in #23 or the one in #36. But custom or contrib modules might have problems when we let any remaining exceptions bubble up, so we should probably not back-port this change to 10.0.x.

Let's update the title to match what we are actually doing.

It seems possible that someone could have committed broken code, never fixed it, and that code could then start throwing exceptions once the site updates to the version that has this in. Good that they'll find out, but could be disruptive.

I think that might be OK in a minor, however we should add a release note and change record (previously swallowed errors will now throw an exception).

Also should the method phpdoc be updated to include @throws?

Will get a second opinion from another committer on whether we need to go further and log the exception/trigger_error() before making a hard fail in Drupal 11.

Re #42 it's also possible someone is relying on the current NULL return to detect that a date cannot be parsed. This is effectively a change in return value and as per policy we should not change these if possible → . In this case we can notify users with @trigger_error that this behaviour will change in future?

I also note that \Drupal\Core\Datetime\Element\Datetime::valueCallback() has similar code:

      try {
        $date_time_format = trim($date_format . ' ' . $time_format);
        $date_time_input = trim($date_input . ' ' . $time_input);
        $date = DrupalDateTime::createFromFormat($date_time_format, $date_time_input, $element['#date_timezone']);
      }
      catch (\Exception $e) {
        $date = NULL;
      }

Should we consider changing this one as well?

This issue appears to stem from the fact that PHP's \DateTime::createFromFormat() does not throw exceptions and returns FALSE on failure, but our DateTimePlus changes this to use exceptions; however in some cases we just revert that behaviour back to the PHP way.

In this case we can notify users with @trigger_error that this behaviour will change in future?

Yeah I'm leaning this way too, although I wonder if we should do trigger_error('...', E_USER_WARNING); so that people actually get an actionable error message in Drupal 10, which can then be an exception in Drupal 11, rather than a silenced deprecation.

@Ranjit1032002 thanks for the patch but it does not apply - it also looks incorrectly formed.

I think the next steps here are to discard the patch so far and change the todo to a trigger_error(), so we can notify users that this behaviour will change. We also need a change record documenting this.