Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#set_recurring_event:` fails for monthly events starting at 30 and 31 #3619

Closed
kghbln opened this Issue Jan 15, 2019 · 11 comments

Comments

Projects
None yet
2 participants
@kghbln
Copy link
Member

commented Jan 15, 2019

Setup and configuration

  • MediaWiki | 1.32.0 (47564a4)01:12, 12 January 2019
  • PHP | 7.0.33-0+deb9u1 (apache2handler)
  • MariaDB | 10.1.37-MariaDB-0+deb9u1
  • Semantic MediaWiki | 3.1.0-alpha (40f720a) 09:40, 14 January 2019

Issue

Refs: #3541

This issue is not entirely a surprise. As already assumed with the referenced issue it is not possible to create recurring monthly events for the 30th and 31st. It is however no regression from 3.0.0 but an issue with the original implementation from 1.5.0.

Steps to reproduce

{{#set_recurring_event: Is team meeting 30 |property=Has date |start=30 janvier 2003 9:30 am |end=31 décembre 2004 |unit=month }}

or

{{#set_recurring_event: Is team meeting 31 |property=Has date |start=31 janvier 2003 9:30 am |end=31 décembre 2004 |unit=month }}

will error out with the following improper value statements:

Unable to interpret the "32202-2-31" input value as valid date or time component with "Month 2 in year 32202 did not have 31 days in this calendar model." being reported.
Unable to interpret the "2003-2-30 09:30:00" input value as valid date or time component with "Month 2 in year 2003 did not have 30 days in this calendar model." being reported.
Unable to interpret the "32202-2-30" input value as valid date or time component with "Month 2 in year 32202 did not have 30 days in this calendar model." being reported. 

See also sandbox

@kghbln kghbln added the bug label Jan 15, 2019

@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

As discussed the expected fix creates a recurring event for the last day of the month if the respective month does not have a 30th or 31st date. See #3598 (comment).

kghbln added a commit that referenced this issue Jan 15, 2019

Provide test for end of month recurring events
This commit is to provide the integration test for the issue reported with #3619 and fails since currently not fix is available.

- Testing the 29th - passing
- Testing the 30th - failing
- Testing the 31st - failing

[skip ci]
@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

This commit 65693d4 is to preserve the integration test I created. It does however NOT assume a timely fix for the issue undetected since 2010-03-07.

@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@mwjames

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

It is however no regression from 3.0.0 but an issue with the original implementation from 1.5.0.

Lines from 1.8.0.5:

// If the date is February 29, and this isn't
// a leap year, change it to February 28.
if ( $cur_month == 2 && $cur_day == 29 ) {
if ( !date( 'L', strtotime( "$cur_year-1-1" ) ) ) {
$cur_day = 28;
}
}

What about changing the following lines?

@@ -281,16 +281,15 @@ class RecurringEvents {
 					$cur_year += (int)( ( $cur_month - 1 ) / 12 );
 					$cur_month %= 12;
 					$display_month = ( $cur_month == 0 ) ? 12 : $cur_month;
 				}
 
-				// If the date is February 29, and this isn't
-				// a leap year, change it to February 28.
-				if ( $cur_month == 2 && $cur_day == 29 ) {
-					if ( !date( 'L', strtotime( "$cur_year-1-1" ) ) ) {
-						$cur_day = 28;
-					}
+				// If the date is greater than 28 for February, and it isn't
+				// a leap year, change it to be a fixed 28 otherwise set it to
+				// 29 (for a leap year date)
+				if ( $cur_month == 2 && $cur_day > 28 ) {
+					$cur_day = !date( 'L', strtotime( "$cur_year-1-1" ) ) ? 28 : 29;
 				}
@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2019

@mwjames I have just done a field test. This works for the 30th but not for the 31st which stops creating events at the first month with only 30 days.

@mwjames

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

30th but not for the 31st which stops creating events at the first month with only 30 days.

How about?

+				if ( $cur_month == 2 && $cur_day > 28 ) {
+					$cur_day = !date( 'L', strtotime( "$cur_year-1-1" ) ) ? 28 : 29;
+				} elseif ( $cur_day > 30 ) {
+					// Check whether 31 is a valid day of a month
+					$cur_day = ( $display_month - 1 ) % 7 % 2 ? 30 : 31;
 				}
@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

How about?

Affirmative, this works! However while testing another issue came to light which will also make the proposed integration test fail: The end date set with end is not being included in the series of recurring events. Again this is no regression but I think it is a bug. Since start is being included one would expect that end is, too. Should I file a report for this or is there a quick and dirty fix?

@mwjames

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

The end date set with end is not being included in the series of recurring events. Again this is no regression but I think it is a bug. Since start is

Unless I'm not mistaken, you have defined it with [0] (end without a time) now if you however, do specify a time let's say 31 December 2004 09:31 it should include that date. The issue you encountered is that of two different ranges with the end not being part of it because of its default 00:00 time. If you take away the time component, you'll see 31 December 2004 to be included.

[0] {{#set_recurring_event: Is team meeting 31 |property=Has date |start=31 janvier 2003 9:30 am |end=31 décembre 2004 |unit=month }}

@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

You are absolutely right. Indeed this explains it and now we get the expected results. Great! Will do a pull soon.

kghbln added a commit that referenced this issue Jan 17, 2019

@kghbln kghbln added this to the SMW 3.0.1 milestone Jan 17, 2019

@kghbln

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

The docu is updated, too. I am so happy that everything appears to be in fluff here.

@kghbln kghbln closed this in #3622 Jan 17, 2019

kghbln added a commit that referenced this issue Jan 17, 2019

Allows recurring events to start on a 30th or 31st of a month (#3622)
* Allows recurring events to start on a 30th or 31st of a month

Code change as suggested by @mwjames #3619 (comment)

* Provides test for end of month recurring events

kghbln added a commit that referenced this issue Jan 18, 2019

Allows recurring events to start on a 30th or 31st of a month (#3622)
* Allows recurring events to start on a 30th or 31st of a month

Code change as suggested by @mwjames #3619 (comment)

* Provides test for end of month recurring events
@mwjames

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

The docu is updated, too. I am so happy that everything appears to be in fluff here.

Well done. Self-accomplishments are the best form of gratification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.