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.
I have found a case, when an exception is not excluded from the list of dates.
View: https://skitch.com/temaruk/gqh6r/fullscreen
Edit: https://skitch.com/temaruk/gqh6m/fullscreen
Is it by design? I would expect even the starting date excluded if that is set.
( I tried to check how this should work at http://www.kanzaki.com/docs/ical/exdate.html )
Comment | File | Size | Author |
---|---|---|---|
#7 | only_tests.diff | 5.01 KB | PatchRanger |
#7 | date-fix_date_exclusion-1365516-7.patch | 7.13 KB | PatchRanger |
#3 | date-fix_date_exclusion-1365516-3.patch | 9.47 KB | PatchRanger |
#2 | date-fix_date_exclusion-1365516-2.patch | 2.29 KB | PatchRanger |
Comments
Comment #1
KarenS CreditAttribution: KarenS commentedThat seems a little ambiguous. I guess it means we should remove it from the array of results, but it's a little vague.
Comment #2
PatchRanger CreditAttribution: PatchRanger commentedConfirmed. I've made tests for this issue. Should fail by 'Exceptions work as expected' assertion.
Please note that test includes testcase for Date API calculations from original topic starter message - it passes. It makes me think that the problem is inside form submission handler, it looks like it incorrectly converts result of form submission to Date API call.
Am going to continue debugging.
Comment #3
PatchRanger CreditAttribution: PatchRanger commentedHere is working solution.
It fixes date exception handling.
It does not deal with start date exception due to ambiguity in specification (EDIT: see comment below).
Patch includes full test coverage: tests break before patch and pass after applying.
Drupal.org testbot is currently stuck for Date-7.x-2.x: see http://qa.drupal.org/pifr/test/200603. I've submitted the patch, that fixes it: #1835214: Automated tests failing - Exception thrown in Date2 migration.
As there is no chance to demonstrate here that this patch really solves this issue and that patch indeed cures testbot, see my special repository at Travis instead: https://travis-ci.org/PatchRanger/drupal7-date/builds/6874960#L233 - all tests passed.
Please review the patch.
Comment #4
PatchRanger CreditAttribution: PatchRanger commentedHere is what I've discovered after further investigation (see http://drupalcode.org/project/date.git/blob/refs/heads/7.x-2.x:/date_rep...):
Any suggestions how to deal with it?
Comment #5
PatchRanger CreditAttribution: PatchRanger commentedLooks related: #1842708: Check that start date matches repeat rules..
Comment #6
PatchRanger CreditAttribution: PatchRanger commentedBy doing 'blame' at this lines of code, I found out that this message about RFC 2445 was added by this commit: http://drupalcode.org/project/date.git/commit/b537326dda467daf935a40f5aa... .
As you could see, it was created - and has never been changed since. In fact, it was not just created - it was moved from date_repeat.module: see http://drupalcode.org/project/date.git/blobdiff/f8a1f25d967ba85eea5e57e2... . Here are the most important lines of it:
So, it appears, that original wording was more accurate and correct. Let's think how it affects our situation.
Comment #7
PatchRanger CreditAttribution: PatchRanger commentedFinally done.
@KarenS Thank you, following your suggestion I've finally got it working.
Only tests and patch+tests are attached.
As they aren't going to be tested by testbot, here are links to Travis environment:
Please note that I am using patch from #1835214: Automated tests failing - Exception thrown in Date2 migration to cure testbot at my Travis environment.
Please review.
Comment #8
Dave Cohen CreditAttribution: Dave Cohen commentedThis other issue is marked a dup of this one: #1842708: Check that start date matches repeat rules.
That other issue refers to the problem where the starting date is always considered a match. While this issue refers to when an exception explicitly includes the start date. Are they really duplicates of one another? I can't tell from just looking at the patch, will this patch exclude the start date when it does not match the criteria, but is not explicitly excluded?
Thanks for any info.
Comment #9
PatchRanger CreditAttribution: PatchRanger commented@Dave Cohen Yes, you were right marking that one as a duplicate, because this patch really will exclude the start date when it does not match the criteria, but is not explicitly excluded: see these lines of the patch:
See? It corrects existing test, making it to behave as I told above: start date is excluded without explicit exclusion because of not matching the criteria.
Comment #11
ejk CreditAttribution: ejk commentedJust ran into this issue with my code and had to track it down. I'm going to have to add custom handling outside of calling this function to work around it.
I don't see how there's any confusion between what data fields need to be maintained in the VEVENT object and what dates are actually calculated from that data. If an EXDATE matches the DTSTART time, it should be excluded from the results.