-
Notifications
You must be signed in to change notification settings - Fork 822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MNT Fix unit tests #11409
MNT Fix unit tests #11409
Conversation
fb8236e
to
46c4a10
Compare
'31/03/2008', | ||
'Mar 31, 2008', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old date is effective the same as you'd get when using IntlDateFormatter::SHORT
, though we're using IntlDateFormatter::MEDIUM
here.
Steve and I had talked in person about making the default configurable.
That would be doable for DateField_Disabled
in the above test, because the default for that is returned from DateField::getDateLength()
as a fallback value. Though its notable that none of the tests for DateField
itself failed... so I'm not sure about the value of making that change but can do if the reviewer wants me to.
That's not possible for DBDate
in this test though, because the default value is a default argument in the method signature of DBDate::getFormatter()
and we can't change method signatures in patches or minors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though its notable that none of the tests for DateField itself failed
Looks like that's because only the internal values were being validated in DateFieldTest, rather than the formatted values like Nice()
so I'm not sure about the value of making that change but can do if the reviewer wants me to.
Reading php/php-src#16127 (comment) particularly around the "no contract" bit, I guess it's correct to not have config here and not attempt to retain backwards compatibility. Simply updating our unit tests is the correct to test the new format is the correct response I think.
a815307
to
1b236ed
Compare
1b236ed
to
ebb0162
Compare
Fixes various date formatting failures in https://github.com/silverstripe/silverstripe-framework/actions/runs/11091228469/job/30830628977
Two main causes:
IntlDateFormatter
output changes based on operating system used php/php-src#16127en_NZ
locale. We shouldn't try to pre-fix the bug, nor should we attempt to revert to the buggy behaviour. Solution is to test against a locale that didn't exhibit the bug.Issue