- Status changed to Needs work
almost 2 years ago 12:43am 20 February 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
-
+++ b/core/lib/Drupal/Core/Datetime/DateHelper.php @@ -453,11 +453,13 @@ public static function ampm($required = FALSE) { + if (!empty($date)) { + if (!$date instanceof DrupalDateTime) { + $date = new DrupalDateTime($date); + } + if (!$date->hasErrors()) { + return $date->format('t'); + } } return NULL;
Let's simplify all of these with an early return
if (empty($date)) { return NULL; }
Instead of the extra nesting
-
+++ b/core/tests/Drupal/Tests/Core/Datetime/DateHelperTest.php @@ -128,4 +159,94 @@ public function providerTestWeekDaysOrdered() { + // December 31st 2022 is a Saturday. ... + // November 30th 2020 is a Monday. ... + // December 31st 2022 is a Saturday. ... + // November 30th 2020 is a Monday. 2020 is a leap year.
Nit: I don't think it being a Saturday, a Monday, or a leap year make to the test of the number of days in a month or year? Let's keep those comments for the tests of the day of the week name/ID, but not for the days in year/days in month tests.
-
- Status changed to Needs review
almost 2 years ago 4:09am 20 February 2023 Addressed Point-1 from Comment #44. Point-2 could have been a little more elaborate to act upon
- Status changed to Needs work
almost 2 years ago 4:14am 20 February 2023 - 🇦🇺Australia acbramley
Changes in #46 are not correct.
+++ b/core/lib/Drupal/Core/Datetime/DateHelper.php @@ -453,11 +453,13 @@ public static function ampm($required = FALSE) { + if (!empty($date)) { + if (!$date instanceof DrupalDateTime) { + $date = new DrupalDateTime($date); + } + if (!$date->hasErrors()) { + return $date->format('t'); + }
All of this logic needs to remain, but instead of wrapping the entire block in the
if (!empty....
You do
if (empty($date))
and return NULL inside that. Then the remaining logic can be underneath that, un-nested. - Status changed to Needs review
almost 2 years ago 4:55am 20 February 2023 - Status changed to Needs work
almost 2 years ago 5:11am 20 February 2023 - 🇺🇸United States smustgrave
Why are we removing the comments now?
Also not the solution has this removed the functionality.
- Status changed to Needs review
almost 2 years ago 9:21am 3 March 2023 - Status changed to Needs work
almost 2 years ago 9:47am 3 March 2023 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇮🇳India TanujJain-TJ
adding a reroll for drupal 10.1.x with simplified return of NULL
- Status changed to Needs review
almost 2 years ago 1:25pm 3 March 2023 - Status changed to Needs work
almost 2 years ago 2:07pm 3 March 2023 The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Status changed to RTBC
over 1 year ago 6:35am 20 March 2023 - Status changed to Needs work
over 1 year ago 6:44am 20 March 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I think this is a behaviour change here.
With HEAD, passing NULL would default to 'now' per the constructor of \DrupalDateTime
e.g.
> \Drupal\Core\Datetime\DateHelper::daysInMonth() DEPRECATED DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated in core/lib/Drupal/Component/Datetime/DateTimePlus.php on line 309. = "31"
So yes, there's a deprecation, but it still defaults to today and outputs 31 (Its March when I wrote this comment).
So I think we probably need a different fix here - one that involves falling back to the current date.
- Status changed to Needs review
over 1 year ago 11:08pm 20 March 2023 - 🇦🇺Australia acbramley
So to keep parity with the current implementation - NULL, FALSE, and an empty string all default to now. 0 and '0' return NULL.
To avoid copying the implementation of each into the unit test (because we would need to construct a DateTime object at 'now' and format it, which then means for things like daysInYear we start to just copy the whole implementation since there would need to be the leap year check as well) I've opted with just making sure those 3 values are NOT null. Then checking for nulls in the other 2, then there is an actual implementation check (i.e a date string gives the correct output).
- Status changed to RTBC
over 1 year ago 12:53pm 21 March 2023 - 🇺🇸United States smustgrave
Change looks good.
There may be a few duplicates of this but I couldn't find them, but feel I saw one earlier.
The last submitted patch, 61: 3281557-61.patch, failed testing. View results →
- 🇺🇸United States benjifisher Boston area
This is unusual: the failing test → is not one of the usual FunctionalJavascript (FJS) tests:
User.Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest ✗ Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1178.xml: PHPUnit Test failed to complete; Error: PHPUnit 9.5.28 by Sebastian Bergmann and contributors. Testing Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest F 1 / 1 (100%) Time: 00:01.530, Memory: 4.00 MB There was 1 failure: 1) Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest::testStub Failed asserting that an object is empty. /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122 /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55 /var/www/html/core/modules/migrate_drupal/src/Tests/StubTestTrait.php:22 /var/www/html/core/modules/user/tests/src/Kernel/Migrate/MigrateUserStubTest.php:35 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728 FAILURES! Tests: 1, Assertions: 8, Failures: 1.
I do not see why it would fail, and it passes when I run the test locally. Neither the user test nor the trait that does most of the work has changed in almost two years.
I am setting the status back to RTBC.
The last submitted patch, 61: 3281557-61.patch, failed testing. View results →
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Re #64 📌 Improve StringItem::generateSampleValue() Fixed this was the cause of the random fail in the migrate test
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Adding credits for @adeshsharma, @TanujJain-TJ and @Rishabh Vishwakarma - whilst their patches were not part of the final solution, they were trying to move the issue forward. Thanks folks, hopefully you picked up some new learning as part of contributing to this issue 💪
-
larowlan →
committed d8436d19 on 10.0.x
Issue #3281557 by smustgrave, ipo4ka704, acbramley, ravi.shankar,...
-
larowlan →
committed d8436d19 on 10.0.x
-
larowlan →
committed 2ce27f62 on 10.1.x
Issue #3281557 by smustgrave, ipo4ka704, acbramley, ravi.shankar,...
-
larowlan →
committed 2ce27f62 on 10.1.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Pushed to 10.1.x and backported to 10.0.x
Waiting for a test run on 9.5.x before backporting there, so leaving at RTBC
-
larowlan →
committed 68c54bf3 on 9.5.x
Issue #3281557 by smustgrave, ipo4ka704, acbramley, ravi.shankar,...
-
larowlan →
committed 68c54bf3 on 9.5.x
- Status changed to Fixed
over 1 year ago 12:31am 29 March 2023 Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
6 months ago 2:29pm 13 June 2024 Can someone explain why we return untranslated string, though the comment says that it should be translated?
/** * Returns translated name of the day of week for a given date. *
return $days[$dow]->getUntranslatedString();
Seems it was brought by these changes.