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

Piwik_ScheduledTime_Monthly - Number of weeks within a month not properly handled #1835

Closed
julienmoumne opened this issue Nov 23, 2010 · 5 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.

Comments

@julienmoumne
Copy link
Member

This bug has been brought on the forum: Topic 15651

In /core/ScheduledTime.php and core/ScheduledTime/Monthly.php I failed to precisely define what is a "week":

  • I didn't define precisely what a week is (is a week defined as starting from Monday? do we account for partial weeks?)
  • I didn't account for the fact that there is a variable number of weeks within a month
  • I didn't account for the fact that there can be more than 4 weeks within a month
  • I didn't think thoroughly whether we needed such a flexible API for monthly scheduled tasks.

From here, there are two ways to go.

Solution 1: Keep the API & Fix edge cases RECOMMENDED

The first solution is to keep the API as it is and add the following fixes:

  1. The following code should schedule the task for the very first day of the month:
$monthly->setDay(1);
$monthly->setWeek(1);

According to the 'day' attribute, setDay(1) means Monday.

If the first day of a month is not a Monday, we simply take the first day of the first week.

Therefore, the 'week' attribute can not define a week starting on Monday. It has to define a partial week.

  1. The following code should schedule the task for the very last day of the month:
$monthly->setDay(7);
$monthly->setWeek(4);

If the last day of a month is not a Sunday, we simply take the closest day to the one specified.

How about if the month has 6 partial weeks? In this case, even though the API user meant to schedule the task for the last day of the month, it wouldn't work.

The only solution I could think of is a comment in the code saying: "To be absolutely certain the task is scheduled for the last day, use ->setWeek(6)".

If a month has less than 6 weeks, we would simple take the last week.

  1. The following code should schedule the task for the Tuesday of the first week of the month:
$monthly->setDay(2);
$monthly->setWeek(1);

What happens if there is no Tuesday the first week of a given month? Do we set for Tuesday of the next week? do we take the first day of the first week?

Those 3 points clearly indicate that keeping this API is a mess from a usability point of view.

Solution 2: Define a reduced API for monthly scheduled tasks [RECOMMENDED]

The API for monthly scheduled tasks should be remade.

Here are two proposals:

  1. Remove setWeek() and use setDay()

In this solution, setWeek() would be removed and setDay() would be used with integers comprised within 1 and 31.

To schedule a task for the very last day of a month, we would use setDay(31) and pick the closest day for months with less than 31 days.

  1. Remove setDay() and setWeek(), add runFirstDay(void) and runLastDay(void)

To schedule the task for the first day, runFirstDay() would be called, to schedule the task for the last day, runLastDay() would be called.

Please comment on the best way to go.

@julienmoumne
Copy link
Member Author

When this bug is fixed, TODO: 1837

@julienmoumne
Copy link
Member Author

setWeek will be removed and setDay will be used with integers comprised within 1 and 31

@julienmoumne
Copy link
Member Author

(In [3364]) fixes #1835, setWeek removed, setDay updated

@mattab
Copy link
Member

mattab commented Nov 26, 2010

(In [3368]) Refs #1835
Using date() as it is built-in (VS cal_days_in_month which requires library)
Unit tests pass so I think it is similar?

@mattab
Copy link
Member

mattab commented Nov 26, 2010

(In [3369]) Refs #1835 update comment as per code update

@julienmoumne julienmoumne self-assigned this Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

2 participants