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.
The data handlers for sure need some proper testing.
Related issues: #1893906: Move views argument date handlers from node module
Comment | File | Size | Author |
---|---|---|---|
#18 | drupal-1934558-18.patch | 13.36 KB | dawehner |
#18 | interdiff.txt | 971 bytes | dawehner |
#15 | drupal-1934558-14.patch | 13.36 KB | dawehner |
#15 | interdiff.txt | 675 bytes | dawehner |
#10 | drupal-1934558-10.patch | 13.49 KB | dawehner |
Comments
Comment #1
dawehnerOne test breaks at the moment, but the rest are working fine.
Comment #3
dawehneryeah data, date it's all the same.
Comment #4
dawehner#1: drupal-1934558-1.patch queued for re-testing.
Comment #6
dawehnerNew version, which might not conflict with the testbot? (no idea why this is needed).
Comment #8
olli CreditAttribution: olli commentedThis looks great.
I think this can be removed too once the handlers are moved to views.
executeView($view, array(52))
Comment #9
olli CreditAttribution: olli commentedoops
Comment #10
dawehnerOh gosh, I searched already for ages. Thanks for open my eyes!
Comment #12
olli CreditAttribution: olli commented#10: drupal-1934558-10.patch queued for re-testing.
Comment #13
olli CreditAttribution: olli commentedIs this needed?
Comment #14
damiankloip CreditAttribution: damiankloip commentedAgreed, this looks like great coverage!
I guess this depends on whether this or the actual moving of the handlers gets in first.
Also, I think olli is right, that usedPlugin property doesn't seem to be used anywhere.
Comment #15
dawehnerYeah it's totally useless.
@damiankloip
Yeah you are totally right, and I'm happy to reroll this patch once your other patch got in.
Comment #17
olli CreditAttribution: olli commentedNot sure why but week is now 01-53, so '02' and '05' here. Or should we ensure the padding elsewhere?
Comment #18
dawehnerYou are awesome man!
Just wondering whether we should fix the behavior in another issue? I guess for users it's probably expected that both "02" and "2" works,
but yeah this is probably out of scope of this issue.
Comment #19
aspilicious CreditAttribution: aspilicious commentedYeah 02 and 2 should be both accepted. (followup)
Comment #20
dawehnerSo you don't have other points to review? ;)
Comment #21
tim.plunkettLooks good to me. I agree with the change made in #18.
Comment #22
olli CreditAttribution: olli commentedIn the follow-up, I think we would want day and month work the same way as week (both 01 and 1 are good).
Additionally, this issue made me wonder if we need to add a year handler "Year for the week" that can be used with the week handler and support sunday as the first day of week as well.
Comment #23
webchickYay, test coverage!
Committed and pushed to 8.x. Thanks!