HostEntityInterface should implement CacheableDependencyInterface

This is a very good call. Will work on it.

This is ready for review again

Sorry to be slow in reviewing this. A few points:

1. In RegistrationType, shouldn't the workflow be able to affect cache context and expiry as well as tags? I'd be tempted to add the workflow as a cacheable dependency in the constructor.

2. In modules/registration_inline_entity_form/registration_inline_entity_form.module you didn't remove a now redundant addition of the entity as a CacheableDependency.

3. In HostEntity you've overridden getCacheMaxAge() in a way that stops us inheriting any lesser max age from a cacheable dependency.

4. The just-in-time updating we're doing in getCacheTags() feels odd. We have to repeat it across getCacheTags, getCacheContexts, and getCacheMaxAge. It's vulnerable to bugs like 2 above. We have to maintain logic that core will take care of for us. But I get your concern about settings not existing yet - I like what you're doing with registration_settings_list. How about if we add all the field, registration type and settings as cacheable dependencies in the HostEntity constructor, but then additionally add the registration_settings_list cache tag if the settings are unsaved? Should work and is neater.

5. addCacheableDependencies() feels like e legacy method that should maybe be deprecated. At the least, it's little confusingly named. But it does contain some interesting ideas ...

6. addCacheableDependencies() adds a registration_list cache tag. Which reminds me that isUserRegistrant(), and all the capacity methods, likely end up depending on registrations. Probably we should add that tag to the host_entity. If we don't, we need to add it to more validators. At the moment only HostHasRoomConstraintValidator has it.

7. addCacheableDependencies() adds session or user.permissions cache contexts. I've no idea why, but if they belong there then probably they belong on the host entity or some of the validators.

Sorry to be slow in reviewing this.

Not a problem, thanks for the review @jonathanshaw!

1. In RegistrationType, shouldn't the workflow be able to affect cache context and expiry as well as tags? I'd be tempted to add the workflow as a cacheable dependency in the constructor.

Technically yes. Workflow doesn't have anything except tags by default I believe, but if someone overrides, they should reasonably expect for those to be picked up. Will fix.

2. In modules/registration_inline_entity_form/registration_inline_entity_form.module you didn't remove a now redundant addition of the entity as a CacheableDependency.

I saw that one myself and left it in for symmetry with the other code path. But I think you are right to remove it. Will fix.

3. In HostEntity you've overridden getCacheMaxAge() in a way that stops us inheriting any lesser max age from a cacheable dependency.

Good catch, I will fix.

4. The just-in-time updating we're doing in getCacheTags() feels odd.

That's why those functions exist in the interface, so I'm using the standard pattern, look at block plugins in the core system module etc. Putting too much in constructors means as the dependent objects change, those changes are not reflected in the cacheability. In terms of performance, the assumption is the objects are cached, so the just-in-time aspect should be called somewhat sparingly. For example, an access result could be cached for days or longer.

5. addCacheableDependencies() feels like e legacy method that should maybe be deprecated. At the least, it's little confusingly named.

It is modeled after renderer::addCacheableDependency, but takes more than one object, which is nice for the caller. I'll add some more flavor to the doc block for that in the interface.

6. addCacheableDependencies() adds a registration_list cache tag...Probably we should add that tag to the host_entity.

I can't have access checks for every host entity page recalculating every time a registration is added. On a busy site that will essentially disable caching. It is there for render arrays. isAvailableForRegistration includes it through the HasRoom validator, which is important. Technically, it should be added to the registration version of capacity and registrant validators - although those dependencies aren't used anywhere that I can think of right now. Nevertheless I will add them, with a note that using those validators, and thereby adding registration_list, to anything that could be used for access control, is terrible for performance. I will also add some code to the cache tests in RegistrationAccessTest making sure registration_list is not in there.

7. addCacheableDependencies() adds session or user.permissions cache contexts. I've no idea why, but if they belong there then probably they belong on the host entity or some of the validators.

They are still there for BC. Originally this method was used by RegisterForm, but now that class has its own code that adds them with a comment "The registrant options depend on user permissions or anonymous session" to explain why. So I would have liked to take them out, but this is a public method, people may be relying on that behavior. This could be a deprecation in the next major upgrade perhaps.

I will make all the changes per above within the next couple days. Planning to release 3.4.0 next weekend. Thanks again @jonathanshaw!

john.oltman → changed the visibility of the branch 3497735-refinable to hidden.

john.oltman → changed the visibility of the branch 3497735-refinable to active.

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