When adding/updating a subscription through the UI (/admin/braintree-cashier/subscription/add), and the subscription's period_end_date field is left empty, that field is always set to the current time. Looks like this is due to what happens in TimestampDatetimeWidget::massageFormValues. See https://drupal.stackexchange.com/a/220961 for more background. The other timestamp fields on the subscription entity don't have this issue, since they're currently not exposed in the form.

Looks like proper way to resolve this is to switch the Timestamp fields to Date fields, since they do support empty values, and have built-in formatters. We could also implement our own timestamp form widget (so we can avoid setting a default timestamp value), but that seems like a temporary fix.

What do you think?

Comments

brunodbo created an issue. See original summary.

brunodbo’s picture

brunodbo
he/him
commented
Issue summary: View changes
shaundychko’s picture

shaundychko
Location Vancouver, Canada
commented

Yes, Drupal's timestamp field UI is a pain. From what I understand, changing it to a Date field would involve an update hook that

  1. temporarily stores the current values in the key-value store
  2. deletes the value in the field in each subscription entity
  3. deletes the field definition, which won't throw an exception since no data is stored any longer
  4. creates the new Date field
  5. repopulates the new field with data stored in the key-value store
  6. deletes the temporary key-value store

Am I missing anything? Since this is a fair bit of hassle, would it suffice to do this only for the "Period end date" field that has a user facing UI? The other date fields are set only programmatically. Since I'm accustomed to working with timestamps, and the code already assumes the period end date is a timestamp, I'm considering keeping the getter/setter for this field as giving/providing timestamps, and just changing the implementation of those methods to consume and emit timestamps for Date fields. This would avoid having to test and modify code elsewhere, and takes advantage of one of the main reasons for using getter/setter functions, no?

If I'm missing anything, please let me know.

  • ShaunDychko committed 1753f59 on 8.x-2.x 
    Issue #3021594 by brunodbo, ShaunDychko: Replace timestamp fields with...
shaundychko’s picture

shaundychko
Location Vancouver, Canada
commented
Status: Active » Fixed

The commit here updates the period_end_date field to the core base type datetime, copying all old values to the new field using the expirable key-value store.

brunodbo’s picture

brunodbo
he/him
commented

Thanks for this!

Yep, I think we can leave the non-exposed timestamp fields for now. If one day we end up needing to show those in the form, we can deal with it then. Though I can't think of a reason to expose them right now, since they are meant to track things internally.

One thing I was wondering about this change:

diff --git a/braintree_cashier.module b/braintree_cashier.module
index 6a23ce9..7025246 100644
--- a/braintree_cashier.module
+++ b/braintree_cashier.module
@@ -186,20 +186,19 @@ function braintree_cashier_cron() {
   // the period end date is set. Subscriptions which will be canceled by a
   // webhook from Braintree will not have a period end date set, and will not
   // be included here.
-  $subscription_ids_to_cancel = \Drupal::entityQuery('subscription')
+  $subscription_ids_will_cancel = \Drupal::entityQuery('subscription')
     ->condition('cancel_at_period_end', TRUE)
     ->exists('period_end_date')
-    ->condition('period_end_date', time(), '<')
     ->condition('status', SubscriptionInterface::ACTIVE)
     ->execute();
 
   /** @var \Drupal\braintree_cashier\SubscriptionService $subscription_service */
   $subscription_service = \Drupal::service('braintree_cashier.subscription_service');
 
-  $subscriptions = Subscription::loadMultiple($subscription_ids_to_cancel);
+  $subscriptions = Subscription::loadMultiple($subscription_ids_will_cancel);
   foreach ($subscriptions as $subscription) {
     /** @var \Drupal\braintree_cashier\Entity\SubscriptionInterface $subscription */
-    if (!$subscription_service->isBraintreeManaged($subscription)) {
+    if (!$subscription_service->isBraintreeManaged($subscription) && $subscription->getPeriodEndDate() < time()) {
       $subscription->setStatus(SubscriptionInterface::CANCELED);
       $subscription->save();
     }

Instead of checking whether $subscription->getPeriodEndDate() < time() in the foreach loop, would it make sense to compare time() formatted as DateTimeItemInterface::DATETIME_STORAGE_FORMAT with the period_end_date value in the entity query condition, in order to minimize the amount of entities to load? Though you would need to have a lot of subscriptions for this to become a scalability problem...

shaundychko’s picture

shaundychko
Location Vancouver, Canada
commented

Thanks for the review!

Regarding the period_end_date condition, I would agree with your suggestion, but I don't understand well enough how the database query does the string comparison. So long as DateTimeItemInterface::DATETIME_STORAGE_FORMAT is the same as ISO8601 format, I suppose string comparison works, but does the DB query do this the way we expect? I have no hesitation comparing integers, which is why I went with the $subscription->getPeriodEndDate() < time() solution, despite having to process that on every cron run for every active subscription set to cancel at period end. If this might need changing, let's please open a new issue and someone can perform due diligence on the concept.

Status: Fixed » Closed (fixed)

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