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.
Follow-up to #2161337: Add a Date Range field type with support for end date
Problem/Motivation
- DateTimeFieldTest and DateRangeFieldTest have a lot of similar/duplicate code.
- When in the future, test coverage is added/changed in DateTimeFieldTest, chances are a similar change will be needed in DateRangeFieldTest too.
Proposed resolution
Create a base class for the two, so that some future changes only need to be made in one place.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 5.94 KB | claudiu.cristea |
#14 | 2786583-14.patch | 18.77 KB | claudiu.cristea |
Comments
Comment #2
mpdonadioWe may want to merge this with #2780063: Convert web tests to browser tests for datetime and datetime_range modules and do both at the same time. This may depend on how some of the new traits on BTB go. There are still a bunch missing to make this an easy conversion.
Comment #3
xjmComment #4
xjmRegarding #2, I recommend not blocking things on a BTB conversion. See my feedback on the BTB meta for why we can't convert individual module tests to BTB at this time: #2735005-69: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)
Comment #5
claudiu.cristeaPatch (not converted to BTB).
Comment #7
claudiu.cristeaComment #9
claudiu.cristeaForgot the @group in base class.
Comment #12
claudiu.cristeaComment #13
jhedstromThis is looking really great! Thanks @claudiu.cristea!
I'm not clear on if this can go into 8.2 or not, but we'll leave it there for now.
A couple of small changes and it is looking good.
While we're refactoring this, let's remove this incorrect comment (this goes back to when the timezone loop was added, but the comment was never cleaned up).
Can we add this property to the base class instead, and have the datetime field test use that as well? (It currently uses
\Drupal::service('date.formatter')
.)Comment #14
claudiu.cristeaThank you @jhedstrom
Here are the changes.
Comment #15
jhedstromThis looks great, thanks @claudiu.cristea!
I am still uncertain which branch it can go into.
Comment #16
jhedstromBumping up to 8.3.x, as it will need to go in there before it goes into 8.2.x (if it can).
Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commented@xjm pointed me to https://www.drupal.org/core/d8-allowed-changes#rc which says testing improvements (i.e., patches that solely touch tests) can go into RCs (and therefore also into betas), so tagging as such, and all 8.x issues are presumed to need to be committed to the latest branch first, and then cherry-picked to the issue's version, so setting that accordingly.
She also mentioned that not every testing improvement is allowed into betas/rcs/patch-releases. For example, #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) is too disruptive. But I don't see anything that disruptive in this patch.
Comment #20
xjmNice cleanup! Committed f03979c and pushed to 8.3.x and 8.2.x. Thanks!
Fixed the following coding standards violation on commit:
Comment #22
jhedstromBack to fixed :)