Add a group formatter for easier subscribe/ unsubscribe

[amitaibu]

Continue work from amitaibu#40

[pfrenssen]

I have reviewed around half of the PR now. I have also tested it, but I seem to encounter some problems.

• I was repeatedly joining and leaving a test group and I had some instances where I got an access denied on the unsubscribe route. I think maybe this was because I had the "pending" state and the unsubscribe access check does not check for this.
• I had an instance where I was no longer a member of the group, and the unsubscribe link was still shown. This might have been a caching issue.

I have to stop for today, work is calling. Will continue review as soon as I find some time.

[pfrenssen]

So the extensive test coverage has uncovered a bug in Drupal core regarding upcasting of entities with an invalid entity type ID. Created bug report: https://www.drupal.org/node/2786897

[amitaibu]

Thanks for the 1st half of the review! (And thanks for doing the code changes)

Cool, I didn't know you can upcast by entity:* - very useful. Also happy to see we reveal some core bugs - it means we are doing some nice stuff 😄

[amitaibu]

Tests seem to fail over new code sniffer fixes. I suspect these are new sniffer rules :/

[amitaibu]

I'll lock the PHPCS version in travis, as fixing "new" sniffer errors is out of scope for this PR.

[amitaibu]

I'll work on adapting the unit tests that are now failing due to the new injections

[pfrenssen]

Yeah the code sniffer was set to the latest dev version, that has a potential to break builds on a daily basis if they make new updates. I had also identified this and addressed it in amitaibu#301

It's good to have it in here too though.

[sandervd]

Won't this bubble up, and make the whole entity view un-cachable?

[amitaibu]

I'm not sure - and I actually don't know exactly how to verify it. Need to do some digging 😉 @pfrenssen do you happen to know how to check it?

[pfrenssen]

Very good find!

I just checked it and this indeed bubbles up to make the entire page uncacheable. To solve it correctly we should use a cache context for the users status in the group, but we don't have this yet. For the moment, let's vary the cache context by user - that is a bit too broad but it will work for now without killing caching. Let's be sure to leave a todo in the code too so we remember to address this properly once we have an OgMembershipStatusesCacheContext. We should also take care to clear this cache when the membership status changes.

Now, how to check cacheability. The easiest way is by enabling the Dynamic Page Cache module and setting up the cacheability debug headers:

1. Set up the site in "Development mode" - copy sites/default/default.services.yml to sites/default/services.yml and edit it. Set the option http.response.debug_cacheability_headers to true.
2. Enable the Dynamic Page Cache module.
3. Clear the cache.
4. Refresh the entity view page of the group and inspect the HTTP headers (for example using the dev tools in Chrome or Firefox). Check the header X-Drupal-Dynamic-Cache - this should show either MISS (for a cold cache) or HIT (for a warm cache). It should not show UNCACHEABLE.

Currently with the cache max-age set to 0 this displays UNCACHEABLE. If I remove the line, the first time is says MISS, the second time HIT.

Unfortunately the dynamic page cache module also considers render elements that vary by the user or session to be UNCACHEABLE, so changing it to this also renders the page uncacheable:

    $elements['#cache']['contexts'][] = 'user';

This is due to the default settings for auto-placeholdering, also in sites/default/services.yml:

    auto_placeholder_conditions:
      # Max-age at or below which caching is not considered worthwhile.
      #
      # Disable by setting to -1.
      #
      # @default 0
      max-age: 0
      # Cache contexts with a high cardinality.
      #
      # Disable by setting to [].
      #
      # @default ['session', 'user']
      contexts: ['session', 'user']
      # Tags with a high invalidation frequency.
      #
      # Disable by setting to [].
      #
      # @default []
      tags: []

So with these default settings if the cache context is set to 'user' then the page will not be cached. This is because it is considered too granular.

I think maybe we should split this up to a followup. We need to make this OgMembershipStatusesCacheContext to make this work properly. I would suggest to make this a critical issue, we should definitely not release with uncacheable group pages :)

[amitaibu]

Thanks for the great explanation on how to check the cache!

I've opened #150. For now I think we should go with the no cache option, as the user cache context won't work - since the cache is indeed based on the OgMembeship and not on the user.

So adding the proper OgMembership cache context would be done in a follow up.

[amitaibu]

(also, re-rolled)

[jhedstrom]

The failing test appears to be just due to a missed instance of GroupManager in commit 0bd2a8b.

[amitaibu]

Correct, thanks. I've converted another old method call to groupTypeManager.

[amitaibu]

I'll merge this tomorrow, as I'd be happy to get those basic things in. We can always revisit as needed.

[amitaibu]

Thanks all!