The problem

Right now License depends on the recurring_period module to calculate fixed or rolling dates based on an interval/
Meanwhile, recurring does its own logic directly in the billing schedule plugins.
There was an idea to make Recurring use the RecurringPeriod plugins, but the BillingSchedule plugins do more than just generate intervals, making this hard. Furthermore, I'm not a fan of having another module to depend on and maintain.

While reviewing License and Recurring I found a bunch of shared bugs caused by PHP's crappy date handling (day overflows, timezone handling, etc). That means it's still desirable to somehow unify the core code.

The solution

License and Recurring have one common dependency: Commerce.

So let's fix this problem here.

We need a value object that takes a number and a unit (hour/day/month/year), and provides the following methods:
- floor(DrupalDateTime $date) - round down to the nearest boundary.
- ceil(DrupalDateTime $date) - round up to the nearest boundary.
- add(DrupalDateTime $date) - add the interval to the given date
- subtract(DrupalDateTime $date) - subtract the interval from the given date
Shamelessly stolen from D3 (https://github.com/d3/d3/blob/master/API.md#time-intervals-d3-time).

CommentFileSizeAuthor
#2 2920212-2.patch11.85 KBbojanz

Comments

bojanz created an issue. See original summary.

bojanz’s picture

bojanz commented
Status: Active » Needs review
StatusFileSize
new 2920212-2.patch11.85 KB

In case someone wants to take a look before commit.

This ensures +1 month and -1 month work like people would expect it to.
It also fixes the week intervals that were broken in Recurring.
Plus, it ensures that the times are reset properly.

  • bojanz committed dca61aa on 8.x-2.x 
    Issue #2920212 by bojanz: Add an Interval value object
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Resolved with some phpcs fixes.

joachim’s picture

joachim commented

There's a fundamental problem with adding a month to periods: what should be the result of Feb 28 + 1 month? You can't know whether it's:

- Mar 31, because the 28 is 'the end of the month', because the previous period was on Jan 31
- Mar 28, because your renewal date is actually the 28th of the month.

Given that, my thinking has been that the recurring period plugin needs to have the original start date, as well as the current date we're working from, and possibly calculate from the original date each time.

bojanz’s picture

bojanz commented
Status: Fixed » Needs work

Thank you for raising a case that I didn't consider.

Feb 28th + 1 month should always give March 28th. That's how rolling cycles work, the day of the month is always preserved, unless it doesn't exist (in which case the last day of the month is taken instead). To always target the end of the month, one would use a fixed cycle.

This is also how MySQL behaves:

mysql> SELECT DATE_ADD( '2017-02-28', INTERVAL 1 MONTH );
+--------------------------------------------+
| DATE_ADD( '2017-02-28', INTERVAL 1 MONTH ) |
+--------------------------------------------+
| 2017-03-28                                 |
+--------------------------------------------+
1 row in set (0,00 sec)
bojanz’s picture

bojanz commented
Status: Needs work » Fixed

Actually, the current code works. I just added the additional test cases: http://cgit.drupalcode.org/commerce/commit/?id=e833edf

joachim’s picture

joachim commented

> Feb 28th + 1 month should always give March 28th.

No, that's not always the case.

Consider:

Alice buys a monthly sub on Jan 28. She expects it to be renewed on the 28th of every month: Jan 28, Feb 28, Mar 28.

Case 2:

Bob buys a monthly sub on Jan 31. He expects it to be renewed on the 31st of every month, or the last day of the month in months without a 31: Jan 31, Feb 28, Mar 31, etc... If Bob's sub switches to the Mar 28, then his 12 months will be 3 days short, because it will go round to Dec 28, Jan 28.

Hence why I think this requires more than just decent non-buggy date calculations. I think when we calculate the next period, we actually need the original start date as well as the current period start.

bojanz’s picture

bojanz commented

@joachim
You are right about the second case. Passing the license/subscription start date to the plugin responsible for generation would allow this case to be handled. There might be a helper we can add to Interval for this, but we'd need to figure out how it would look.

It's interesting that this issue was never raised or solved in D7. Shows that Rolling cycles were both less used and less tested.

joachim’s picture

joachim commented

> Shows that Rolling cycles were both less used and less tested.

This would come up with both fixed and rolling cycles. It depends on the start date set by the store admin, or the day the user buys the subscription, respectively.

Is it maybe rather that most places do annual subs rather than monthly?

> Passing the license/subscription start date to the plugin responsible for generation would allow this case to be handled. There might be a helper we can add to Interval for this, but we'd need to figure out how it would look.

Well this is why I was going for a common plugin to handle this... I hadn't yet done work on it to pass in the start date because I was waiting for Commerce Recurring work to start, to see what that would need, and if there were any use cases I hadn't considered.

bojanz’s picture

bojanz commented

This would come up with both fixed and rolling cycles. It depends on the start date set by the store admin, or the day the user buys the subscription, respectively.

No, cause fixed cycles always start at the beginning of the period and end at the end. Beginning of the month - end of the month.
And fixed cycles were our primary use case in CLB. Which of course doesn't mean we shouldn't find a solution for rolling cycles, it's def important.

joachim’s picture

joachim commented

> No, cause fixed cycles always start at the beginning of the period and end at the end. Beginning of the month - end of the month.

Don't be so sure... the UK tax year starts on the 6th of April each year, so lots of companies that do fixed yearly cycles go with that date. The deadline for my kids' nursery payments is something like the 10th of each month. Etc...

bojanz’s picture

bojanz commented

Sure, but we don't support a fixed billing date that's not the beginning of the period. It's always the 1st of the month.
It's an interesting feature to explore in the future, for parity with commercial solutions, though.

joachim’s picture

joachim commented

Anyway, my point remains:

We need more than just bug-free date calculations, we need something that considers the original start date.
It also needs to be something that's common to both License and Recurring, otherwise users who renew manually and automatically will eventually get out of step and one method will be giving you less time than the other!
Which brings me back to having it as a plugin type in a common module. And having it in a separate project makes sense to me because it'll get it wider use outside of Commerce.

bojanz’s picture

bojanz commented

@joachim
We've turned this issue into a chat room, discussing what is now out of scope for the work that was done. Let's continue on Slack.

joachim’s picture

joachim commented

> Sure, but we don't support a fixed billing date that's not the beginning of the period. It's always the 1st of the month.

I actually have a requirement for fixed years that might start on any month, depending on the product.

  • bojanz committed e833edf on 8.x-2.x 
    Issue #2920212 followup: Expand the test cases.

Status: Fixed » Closed (fixed)

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