CommentFileSizeAuthor
#55 date-n1137062-55.patch830 bytesDamienMcKenna
#27 date_date_week_range_and_date_iso_week_range_both_returns_1_day_too_much-1137062-17.patch1.16 KBEl Alemaño
#27 date_date_week_range_and_date_iso_week_range_both_returns_1_day_too_much-1137062-17.patch1.16 KBEl Alemaño
#27 date_date_week_range_and_date_iso_week_range_both_returns_1_day_too_much-1137062-17.patch1.16 KBEl Alemaño
PASSED: [[SimpleTest]]: [MySQL] 5,337 pass(es). View
#16 date_date_week_range_and_date_iso_week_range_both_returns_1_day_too_much-1137062-16.patch2.97 KBhughworm
PASSED: [[SimpleTest]]: [MySQL] 5,244 pass(es). View
#13 date_date_week_range_and_date_iso_week_range_both_returns_1_day_too_much-1137062-13.patch760 bytesschifazl
FAILED: [[SimpleTest]]: [MySQL] 5,240 pass(es), 4 fail(s), and 0 exception(s). View
#3 1137062_Date_range_returns_1_day_too_much.patch15.29 KBSylvain Lecoy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1137062_Date_range_returns_1_day_too_much.patch. Unable to apply patch. See the log in the details link for more information. View
#1 1137062.patch15.52 KBSylvain Lecoy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1137062.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Sylvain Lecoy’s picture

FileSize
15.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1137062.patch. Unable to apply patch. See the log in the details link for more information. View

Here a patch to fix it.

Also updated the test file but the testing suit seems deprecated ?

Sylvain Lecoy’s picture

Also my eclipse editor is removing trailing space on saving action. So in the patch there is trailing space removed but it is not intended.

Sylvain Lecoy’s picture

FileSize
15.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1137062_Date_range_returns_1_day_too_much.patch. Unable to apply patch. See the log in the details link for more information. View

Oops forgotten to provies good paths. Fixed it

KarenS’s picture

Status: Active » Closed (won't fix)

The function works as intended, your change will break everything in Date and Calendar that is using it. It is finding the outside parameters of the week. And I don't know what you mean by saying the testing suite seems deprecated, the tests are working.

If you see a place where the Date and Calendar code create the wrong results because of a bad value here, please reopen, but you haven't indicated anything is broken, just that the function doesn't produce the results you expected. Since making this change would require changing dozens of other lines of code, I won't apply it unless there is some way that Date and Calendar are broken without this.

Sylvain Lecoy’s picture

Status: Closed (won't fix) » Active

Its not working as expected, while I understand the tight coupling between Date and Calendar. I build up a Calendar module based on the Dape API and I found that the date_week_range return one day too much.

And I don't know what you mean by saying the testing suite seems deprecated, the tests are working.

I couldn't run the test suit (i'm on php5.3).

that the function doesn't produce the results you expected

Not that is only me, but I think the standard week is Monday to Sunday included. Its how they work here: http://www.epochconverter.com/date-and-time/weeknumbers-by-year.php. But maybe having the next Monday included is for some reasons that I don't understand ? If so I would really appreciate if you can explain to me.

Because right now I can't rely on this function or I have to duplicate code to have a correct version. If I provide a patch it has to be for Calendar as well ? How can I do that ? Shouldn't we add then a dependency to Calendar in the Date info file ?

KarenS’s picture

Priority: Critical » Normal

This is in no way a critical issue, leaving aside the question about whether it is even a bug.

pwaak’s picture

This may or may not be related, but I get this after applying the patch for #1227350: Date summary views broken in latest dev.

The weeks are off by two in summary view. I have three nodes with dates in August: 2011-08-01, 2011-08-08, and 2011-08-15. All the times are 10:00 - 12:00 (GMT-5), so midnight is not the issue. When the summary view is set to week granularity, I get

July 24 2011 (29) (1)
July 31 2011 (30) (1)
August 7 2011 (31) (1)

when it should be

August 7 2011 (31) (1)
August 14 2011 (32) (1)
August 21 2011 (33) (1)

However, the argument 2011-W32 returns the node for 2011-08-01. This is only one week off as 2011-08-01 is in week 31. Clicking on the "August 7 2011 (31) (1)" link in the summary returns no results.

Incidentally, I think listing the first day of the week in the summary instead of the last day would be more intuitive for users.

radimklaska’s picture

Hi, I have same problem as described in original post. My view (basic table, not a calendar) takes week as an parameter in this format: "2013-W11".

2013-W11 should be from 4.3.2013 to 10.3.2013 (d.m.Y) but view is also showing 1 extra post (I have one post per day.) with date 11.03.2013.

"Use ISO-8601 week numbers" settings have no effects on the problem.

Can some server side settings affect this behavior? Is there any aditional info I can provide?

hughworm’s picture

Issue summary: View changes

KarenS, you say

> The function works as intended, your change will break everything in Date and Calendar that is using it.
No so. To see this bug (whether it is in Calendar or Date) create a view with a calendar display and a week-date pager; you would expect to see 7 days for each paged week, but you get 8.

I applied the fix suggested here so now I only get the conventional 7 days per week, all else seems to be working as expected.

In addition the comment above the function says "@return A numeric array containing the start and end dates of a week." but as is it returns the start dates of the week and the following week.

It's a bit disappointing that after 3 years I have a new release of Date and have to re-apply this fix. What's the problem?

schifazl’s picture

I agree with hughworm, this doesn't work as intended and it's quite illogical, I'm making a view with a weekly timetable for a school and I get also the next Monday. Changing the "+7 days" to "+6 days" doesn't seem to break anything.

Sylvain Lecoy’s picture

Ask Karen she does not want to fix this and even don't see it as a bug whereas when comparing to http://www.epochconverter.com its clearly a bug.

But this is Drupal and hey you know Drupal is different from standards by design so I guess you'll have to live with it :)

schifazl’s picture

She replied three years ago, so maybe in the meantime she changed her mind :)

Anyway I checked where this function could influence other parts of date/calendar. If we go up the function calls, we can see that the fixed functions are influencing two parts od the date module: the date pager and the theming, so I really don't see any big problems that could arise after this simple fix. Anyway, to be sure, I'm doing some tests and everything works fine for now.

schifazl’s picture

Status: Active » Needs review
FileSize
760 bytes
FAILED: [[SimpleTest]]: [MySQL] 5,240 pass(es), 4 fail(s), and 0 exception(s). View

I suppose that if we're hoping in a fix & commit, we should provide a patch, shouldn't we? ;)

Status: Needs review » Needs work
schifazl’s picture

Checking the tests, they're IMHO not correct:

Test calendar date_week_range(5, 2008): should be 2008-01-27 to 2008-02-03, found 2008-01-27 to 2008-02-02.
Test calendar date_week_range(5, 2009): should be 2009-01-25 to 2009-02-01, found 2009-01-25 to 2009-01-31.
Test ISO date_week_range(5, 2008): should be 2008-01-28 to 2008-02-04, found 2008-01-28 to 2008-02-03.
Test ISO date_week_range(5, 2009): should be 2009-01-26 to 2009-02-02, found 2009-01-26 to 2009-02-01.

The weeks in the tests have eight days.

It's necessary to open a new issue for the tests or everything could be done here?

hughworm’s picture

FileSize
2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 5,244 pass(es). View

Thanks for your time on this schifazl.
I guess we need to patch the tests, so I attach a revised patch file.
It now passes the revised date_api test on my environment...

schifazl’s picture

I did that too, but my question is another:
the tests shoud be fixed in another patch?

In another module's issue queue a patch was failing tests for some errors in the tests. The module's owner first created and committed a patch for the tests, and only after that he re-tested and finally committed the patch.

That's why I have this doubt :)

schifazl’s picture

Status: Needs work » Needs review

No one knows? Well, let's see what happens now...

The last submitted patch, 1: 1137062.patch, failed testing.

The last submitted patch, 3: 1137062_Date_range_returns_1_day_too_much.patch, failed testing.

schifazl’s picture

Test passed! We need some more feedback to set status to RTBC I think (and hope that this patch won't be rejected).

Sylvain Lecoy’s picture

Status: Needs review » Reviewed & tested by the community

Let's go go go !

schifazl’s picture

We must cross our fingers now ;)

podarok’s picture

Status: Reviewed & tested by the community » Fixed

  • podarok committed 964511c on authored by hughworm
    Issue #1137062 by Sylvain Lecoy, schifazl, hughworm: date_week_range and...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

El Alemaño’s picture

Hi,

I think that with this patch, the week is not complete. I think we need to change:

date_modify($max_date, '+6 days');

with:

date_modify($max_date, '+6 days +23 hours +59 minutes +59 seconds');

Otherwise the last day is missing.

Thanks

El Alemaño’s picture

Sylvain Lecoy’s picture

What do you mean by "the last day is missing"; you are testing this module with what ?

El Alemaño’s picture

Hi,
Sorry you are right, I have to give some Information. Sorry about that.
The problem that I have is with a Week View. The View Query was looking like this:

WHERE (( (DATE_FORMAT(ADDTIME(field_data_field_datetime.field_datetime_value, SEC_TO_TIME(7200)), '%Y-%m-%d %H:%i:%s') >= '2015-06-21 00:00:00' AND DATE_FORMAT(ADDTIME(field_data_field_datetime.field_datetime_value, SEC_TO_TIME(7200)), '%Y-%m-%d %H:%i:%s') <= '2015-06-27 00:00:00') )

This is not a week, end should be 23:59:59. That is why I did the patch.

graham.roberts’s picture

I agree with #30.

The original implementation, date_modify($max_date, '+7 days');, creates a period that is a tiny bit too long, for example it would start at 00:00 on Sunday and finish at 00:00 the following sunday, so Sunday is included twice. That would be ok if a less than operator were used, but since a less than or equal to operator is used in the SQL, it does not exclude the beginning of the second Sunday.

But the change date_modify($max_date, '+6 days') means the period runs from 00:00 on say Sunday, to 00:00 on the 6th day, eg Saturday, meaning that the whole of Saturday is excluded.

Either using date_modify($max_date, '+6 days +23 hours +59 minutes +59 seconds'), or changing the SQL condition to use a less than or equal to should solve this.

El Alemaño’s picture

Hi,

@graham.roberts Whats is wrong with my patch #27? I am using it, and works great for me.

Thanks!

Sylvain Lecoy’s picture

Views module is messing up, but hey I think this problem belongs to view because when we are using it as an API it works fine and Views should not dictact API design but instead rely on them.

Eric_A’s picture

Status: Active » Needs review

Patch looks great.

Eric_A’s picture

Only problem is dat the title matches the original issue and not the current follow-up issue.

Eric_A’s picture

Status: Needs review » Reviewed & tested by the community

This follow-up makes total sense.

Nelson Lago’s picture

Version: 7.x-2.x-dev » 7.x-2.9
Priority: Normal » Major

Hi,

To summarize (and check I got this right):

Assuming the week starts at Monday, it is quite reasonable to say that the "last day of the week" is "Sunday"; after all, we are talking about "a day", and the specific point in time that refers to it is "Sunday, 00:00:00h".

But, if we are talking about a "week range", it is quite reasonable to say we are interested in the time interval between the beginning of the first day of the week and the end of the last day of the week; therefore, a "week range" fits more naturally, at least for me, with the idea of "Sunday, 23:59:59h".

If I understand things correctly, the original code would yield an almost correct "week range" (from Monday 00:00:00 to the next Monday 00:00:00) but an incorrect "last week day" (Monday instead of Sunday). But the Calendar module assumed that and things mostly worked ok, because when you ask the API for a range you usually will not worry much about the "last day", only the range itself.

The patch in #16 fixes the "last day" problem but worsens the "wrong range" problem, excluding a whole day from the range. This was fixed by the patch proposed in #27.

The problem is that #16 was included in release 7.x-2.9 but #27 was not. This created a bug in calendar view: https://www.drupal.org/node/1779142 . I believe this is a really serious bug, as it broke a very basic functionality in calendar that worked for ages and affects basically anyone with events in a calendar that are not "all day" events. Given that and the fact that a patch is already available, I believe it warrants a new release.

bjcooper’s picture

Thanks for the summary, @Nelson.

I can confirm that the Date API module release that includes the patch from #16 did cause results from the seventh day of my Calendar Views week displays to be excluded.

I also confirm that the patch from #27 fixed the problem for me.

Without #27, the week-display queries for my Calendar Views clearly show that results must start before the beginning of the last day to be included, effectively leaving off results from the entire last day.

ron_s’s picture

I can also confirm that patch #27 solves our issues with Date 7.x-2.9.

With 7.x-2.8, the WHERE clause of the Views query looks like the following on a Week calendar:

WHERE (( (DATE_FORMAT(ADDTIME(field_data_field_date.field_date_value2, SEC_TO_TIME(-21600)), '%Y-%m-%d %H:%i:%s') >= '2015-12-06 00:00:00' AND DATE_FORMAT(ADDTIME(field_data_field_date.field_date_value, SEC_TO_TIME(-21600)), '%Y-%m-%d %H:%i:%s') <= '2015-12-13 00:00:00') )

While the WHERE clause with 7.x-2.9 generates as follows:

WHERE (( (DATE_FORMAT(ADDTIME(field_data_field_date.field_date_value2, SEC_TO_TIME(-21600)), '%Y-%m-%d %H:%i:%s') >= '2015-12-06 00:00:00' AND DATE_FORMAT(ADDTIME(field_data_field_date.field_date_value, SEC_TO_TIME(-21600)), '%Y-%m-%d %H:%i:%s') <= '2015-12-12 00:00:00') )

In 7.x-2.9, one less day is being retrieved (Dec 12 vs Dec 13). This is causing no entries to be returned for the last day of the week. With the patch applied, we now see the following for the WHERE clause:

WHERE (( (DATE_FORMAT(ADDTIME(field_data_field_date.field_date_value2, SEC_TO_TIME(-21600)), '%Y-%m-%d %H:%i:%s') >= '2015-12-06 00:00:00' AND DATE_FORMAT(ADDTIME(field_data_field_date.field_date_value, SEC_TO_TIME(-21600)), '%Y-%m-%d %H:%i:%s') <= '2015-12-12 23:59:59') )

This is a better result, since a given day should be "less than or equal to 23:59:59", and not "00:00:00" as it was previously.

niallmurphy-ie’s picture

#27 fixes problem for me as well on 2.9.

Sylvain Lecoy’s picture

So you need to use the patch in #27

ron_s’s picture

We have found that patch #27 causes an unintended issue with the Week Calendar template. Please review the Calendar patch I've created, and let me know if there are any questions or feedback. Thanks.

https://www.drupal.org/node/2673192

podarok’s picture

Version: 7.x-2.9 » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs work

according to #42

pf17’s picture

patch # 27 works well for me

ron_s’s picture

Status: Needs work » Reviewed & tested by the community

@podarok, why is this "needs work"? Per my comment in #42, it's simply necessary to include the patch in Issue #2673192. Patch #27 works fine as long as the other patch is committed too.

pslcbs’s picture

patch #27 works great for me.

Thank you very much!!

Michael_Lessard_micles.biz’s picture

Greetings,

I applied the patch (#27) to the Date module 7.x-2.9.

Result : in the week view, an event on Sunday now appears.

BUT multi-column presentation (events lasting many days) stop at Saturday.
( in Views, multi-column is an option, you can switch it to show the event title in every column if you prefer, which would probably bypass this issue remaining after patch #27. )

I much prefer the multi-column display of long events.

ron_s’s picture

@Michael_Lessard_micles.biz, please apply the patch linked in #42. I believe that will solve the issue you're describing: https://www.drupal.org/node/2673192

Both Issue #1137062 and #2673192 need to be committed together.

Michael_Lessard_micles.biz’s picture

+1 I applied both patches (the one here and the one offered by ron_s above).

Both worked seamlessly (no errors).

Calendar is now fully fixed. Thank you.

ronnienorwood’s picture

Patch in #27 works for me.

Thanks !

caldenjacobs’s picture

Patch #27 worked for me as well. Cheers

lunazoid’s picture

We had a calendar with a week display, and the events on the 7th day weren't showing. Applying patch #27 fixed it for us.

merrizervas’s picture

Patch #27 worked for me!

caldenjacobs’s picture

This really should be committed.

DamienMcKenna’s picture

Seems like this one is ready to be included in the next release?

Re-uploading to run it through the latest tests.

DamienMcKenna’s picture

ron_s’s picture

@DamienMcKenna, just to reconfirm per comment #42, this patch will cause a problem in the Calendar module if the patch for Issue #2673192 is not released at the same time.

DamienMcKenna’s picture

@ron_s: Thanks for the clarification. I don't know if any of the Calendar maintainers are overly active..