Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #N
Problem/Motivation
We have no unit test coverage for the date service.
Proposed resolution
Add unit tests for \Drupal\Core\Datetime\Date
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#18 | interdiff.txt | 1.57 KB | dawehner |
#18 | date-2122529.patch | 12.4 KB | dawehner |
#15 | date-2122529-15.patch | 11.41 KB | tim.plunkett |
#15 | interdiff.txt | 1.41 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettI had to inject the config factory and wrap 2 procedural calls in protected methods, but otherwise it was pretty straightforward.
This gives us 100% line coverage of \Drupal\Core\Datetime\Date.
Comment #2
tim.plunkettIn IRC, Paris suggesting specifying the DateTime class via the constructor. Seems reasonable, and removes some extra test code.
Comment #3
tim.plunkettDuh. I should remove this default value in the next reroll... Waiting for the bot though.
Comment #4
dawehnerDid you tried to use the static method using the trick described on http://sebastian-bergmann.de/archives/883-Stubbing-and-Mocking-Static-Me... ?
Comment #5
tim.plunkettThe $dateTimeClass change is not just for testing purposes, it is an actual improvement that will be helpful for anyone swapping out the date service.
But thanks for that link, I will keep that in mind.
And because @dawehner and ParisLiakos both asked, the DrupalDateTime::createFromTimestamp() is a static factory method, so I cannot inject an object.
And in addition, DrupalDateTime extends PHP's \DateTime, so I don't think we should modify its behavior too much to, for example, inject the date service into *it*.
Comment #6
dawehnerWe could make it accept a callable as argument.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedComment #8
dawehnerI agree that this was a bit over the top.
Comment #9
tim.plunkettNeeded a reroll and some new workarounds after #2098089: Date formats cannot be translated (the core UIs are useless).
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedsigh:( so much for getting rid of TestDate in the first place..
looks good to me and i ll rtbc once green
Comment #11
dawehnerReading config_context_enter we should just inject the config.context.factory in here and use it
this is directly $this->configFactory->leaveContext()
Comment #12
tim.plunkettThat's what I wanted the original issue to do, they said it wasn't doable.
I'm glad it is.
Comment #13
dawehnerPerfect, way better!
Comment #15
tim.plunkettIt helps if we actually enter the context.
Comment #16
dawehnerDoes that mean that the unit test does not cover the entering into the config context?
Comment #17
tim.plunkettI have no idea how that would even work.
Comment #18
dawehnerThere we go.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedComment #20
martin107 CreditAttribution: martin107 commentedI am not sure a reroll is appropriate.
Lots has changed since November. This patch was committed. #2111349: Move format_plural to the string translation service and format_interval to the date service.
which notably introduced lots of tests. \Drupal\Tests\Core\Datetime\DateTest is already in core.
Although if people feel strongly the missing goals not covered by the existing tests could be augmented with test from this issue.
Otherwise should this issue be closed?
Comment #21
mikey_p CreditAttribution: mikey_p commentedI think is still useful, as DateTest only tests formatInterval, and not format, which will be needed when format_date is removed. This should probably move the format_date tests over to DateTest.
Comment #22
mikey_p CreditAttribution: mikey_p commentedLooks to me like the tests in core/modules/system/src/Tests/Common/FormatDateTest.php should be moved to core/tests/Drupal/Tests/Core/Datetime/DateTest.php.
Comment #23
mikey_p CreditAttribution: mikey_p commentedIn trying to clean this up I ran into a number of issues, and I think we may want to split up some of the behavior of DrupalDateTime. It's currently hard to test the DateFormatter (and format_date()) since it relies on DrupalDateTime::format(), which in turn relies on things like default/per user time zones and the string translation service. Seems like it would make more sense to break it up so that all the formatting work is done in the date formatting service, and DrupalDateTime just relies on that more testable service. Thoughts?
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThere's not much to reroll here. We'll need to find out what's left to write tests for.
Re #22: the former is a simpletest whereas the latter is a unit test. Merging those is not really an option I think?
Comment #25
mpdonadioI think we can postpone this until we evaluate the state of things as part of #2543958: [META] DateTime Module Improvements.
Comment #26
mgiffordComment #28
jhedstromNot sure what else is to be done here. The class this was opened for has been renamed to
DateFormatter
, which already has extensive unit test coverage in\Drupal\Tests\Core\Datetime\DateTest
.Comment #30
jhedstromClosing this out.