Problem/Motivation

The the static factory methods, when there is a problem parsing input and creating an object, the method throws a generic \Exception. This is too generic, and doesn't allow calling code to distinguish between problems.

Proposed resolution

Use \InvalidArgumentException and \UnexpectedValueException as appropriate.

Remaining tasks

User interface changes

API changes

Different exceptions.

Data model changes

CommentFileSizeAuthor
#9 interdiff-02-09.txt5.58 KBmpdonadio
#9 2830079-09.patch8.72 KBmpdonadio
#2 2830079-02.patch4.46 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Active » Needs review
FileSize
2830079-02.patch4.46 KB

This is going to fail. Run locally to see what is going on

../vendor/bin/phpunit --debug --verbose tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php

Status: Needs review » Needs work

The last submitted patch, 2: 2830079-02.patch, failed testing.

The last submitted patch, 2: 2830079-02.patch, failed testing.

The last submitted patch, 2: 2830079-02.patch, failed testing.

The last submitted patch, 2: 2830079-02.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

What's left to be done to fix the failure?

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

The problem is that the invalid date "23 abc 2012" is failing against the format "d M Y" instead of the sanity check to make sure. This means InvalidArgumentException gets thrown instead of UnexpectedValueException. Fixing this would mean adjusting the test, which I am always hesitant to do. I guess adjusting the provider to provide the exception class would be OK? Will post that later.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Needs work » Needs review
FileSize
2830079-09.patch8.72 KB
interdiff-02-09.txt5.58 KB

OK, this should pass. Will write up a draft CR later.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

I guess adjusting the provider to provide the exception class would be OK?

Given the nature of this issue, that seems completely reasonable to me.

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
@@ -98,11 +98,13 @@ public function testInvalidDateDiff($input1, $input2, $absolute) {
-   * @expectedException \Exception
...
-  public function testInvalidDateArrays($input, $timezone) {
+  public function testInvalidDateArrays($input, $timezone, $class) {
+    $this->setExpectedException($class);

This is good to see too, given the goals of #2822837: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException.

Once the CR is in place, I think this is good to go.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Issue tags: -Needs change record

Ok, draft CR is ready for review.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Reviewed & tested by the community

I reviewed (and added some minor formatting changes to) the CR, and that looks good too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 45d2568 and pushed to 8.3.x. Thanks!

  • alexpott committed 45d2568 on 8.3.x 
    Issue #2830079 by mpdonadio, jhedstrom: Change DateTimePlus to throw...

Status: Fixed » Closed (fixed)

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