RegistrationIsOpen constraint should add cache expiry metadata

.

Setting max age in registration is a bad idea. Hold on this until I can explain further.

I was intentional when designing the existing access control to not add an expiration to the cache (i.e. set a max-age). Setting the max-age would bubble up and affect any page with a Register tab or a Register form, which could be most pages on sites that revolve around events. For example, if we set a max-age of 5 minutes, then all of the host entity pages on a site with the Register tab enabled will need to rebuild every 5 minutes. Site builders have the expectation that the max age set on the Drupal core Performance page will be respected generally, so we should not override that.

When combined with #3497729 📌 Use HostEntity::isAvailableForRegistration in RegisterAccessCheck Active , it creates a performance issue. Availability checks are relatively expensive, since they perform a count query on the registration table. If availability checks are being done for all host entity pages on a site at a regular interval (e.g. every 5 minutes), it creates large and unnecessary database contention on the registration table. This is not desirable, and ignores the event life cycle. Events are by their nature transitory. During the "active" phase, events are accepting registrations, and site owners may be driving a ton of traffic to the host entity pages through ad campaigns and other methods. We want those pages to be really performant, and fully cached, during this phase. Once the events have passed, or registration is full, performance is less of an issue, as site owners are likely to either archive the host pages, or at least disable any ad campaigns. At this point, giving an error if someone attempts to register is not a problem, and site owners can use the "set and forget" option to disable registration entirely if desired.

Reopening, as I just realized I misread your intentions. You don't want a short cache expiration. You just want it around the open/close dates. Sorry for my oversight. Implementing this will be interesting, since the open/close could change.

Yes, that's right.

I think we need to add the settings entity as a cacheable dependency to make sure that changes to the open/close date are considered; as well as the expiry.

Would be great if you could lead on this one @john.oltman.

Ready for review, I believe this is what you had in mind.

I didn't have to add a cache dependency on the host entity settings, since the HostIsOpen constraint has a dependency on the HostHasSettings constraint, which adds it already.

Just one thing:

$storage_timezone = new \DateTimeZone(DateTimeItemInterface::STORAGE_TIMEZONE);
      $now = new DrupalDateTime('now', $storage_timezone);

My understanding of best practice here is to use the date time.time service to get a timestamp of the current time.

Otherwise rtbc.

Switched to time service in the max age calculation. In a couple places dates are being compared so not possible there.

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