Regression: \Drupal\hal\LinkManager\LinkManagerBase implicitly depends on REST module

Darn, good catch :(

This really means \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::SERIALIZATION_CONTEXT_CACHEABILITY should be moved out of rest.module into serialization.module. Darn.

Fortunately, that was newly introduced in 8.5, so we can move it without having to worry about BC, because 8.5 is not even in beta yet.

The best place in existing code for that constant to live is \Drupal\serialization\Normalizer\NormalizerBase, but that feels wrong too, because that's a base class, not an interface. OTOH, we kind of really want you to use that base class for all normalizers anyway… so maybe it's okay.

Alternatively to that, we should add \Drupal\serialization\Normalizer\CacheableNormalizerInterface, which would look like this:

interface CacheableNormalizerInterface extends NormalizerInterface {

 /**
   * Name of key for bubbling cacheability metadata via serialization context.
   *
   * @see \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
   * @see \Symfony\Component\Serializer\SerializerInterface::serialize()
   * @see \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody()
   */
  const SERIALIZATION_CONTEXT_CACHEABILITY = 'cacheability';

}

Thoughts?

Just discussed in IRC:

11:33:17 <damiankloip> timmillwood pong
11:34:00 <timmillwood> damiankloip: apart from the 800+ failing test, any concerns with https://www.drupal.org/project/drupal/issues/2931765?
11:37:11 <damiankloip> timmillwood just the fails mainly :) I would prefer not to do that, but I don't see a better way
11:37:27 <damiankloip> cacheability is quite a tangle anyway
11:38:38 <timmillwood> damiankloip: My concern is calling the interface CacheableNormalizerInterface doesn't make it very flexibly for future additions, should we have NormalizerInterface? Also, should we mark it @internal?
11:40:46 <damiankloip> timmillwood Hmm, what kind of future additions though? wouldn't it just be another interface in that case, if it's not related to cacheability? it's kind of weird having this interface just provide a constant already
11:41:06 <damiankloip> and the interface is not really checked anywhere?
11:41:12 <damiankloip> because it's all in $context
11:41:52 <timmillwood> damiankloip: yeh, it is weird, we could add the constant to NormalizerBase, but isn't that weirderer.
11:42:07 <damiankloip> timmillwood not sure which is weirdererist
11:43:41 <damiankloip> timmillwood and having that constant on the resource response subscirber is kind of strange in the first place too
11:44:23 <timmillwood> damiankloip: how about adding it to serialization.module?
11:44:53 <timmillwood> or serialization.services.yml as a container parameter?
11:45:38 <damiankloip> timmillwood container param sounds like overkill on the work front
11:45:51 <damiankloip> ideally it would be prefixed with an underscore though
11:45:56 <damiankloip> but can't really do that now
11:46:13 <timmillwood> damiankloip: we can, this is 8.5.x pre-alpha.
11:47:18 <damiankloip> timmillwood oh, it was added to the subscriber as a constant in 8.5.x too?
11:47:39 <timmillwood> damiankloip: yes, which is why it's fine to move.
11:47:52 <timmillwood> damiankloip: yes, it was added in https://www.drupal.org/project/drupal/issues/2864816
11:48:06 <damiankloip> timmillwood but can we also change 'cacheability' to '_cacheability' I wonder?
11:48:14 <damiankloip> not strictly a requirement, sorry
11:48:36 <WimLeers> "weirdererist" :P
11:48:42 <damiankloip> :)
11:48:47 <timmillwood> damiankloip: ah, and underscore there.. that I'm not sure we can change.
11:48:54 <damiankloip> yeah...
11:49:05 <damiankloip> would just be nice to mark it as internal of sorts
11:49:12 <WimLeers> timmillwood: container param doesn't work, because then anything using it would need to have this container param injected
11:49:14 <damiankloip> as $context is a complete wild wild west
11:49:20 <WimLeers> creative thinking though :D
11:49:29 <damiankloip> yeah, container param would cause a load of effort
11:49:40 <damiankloip> we don't like too much effort just before christmas timmillwood :P
11:49:59 <WimLeers> The problem is indeed the Symfony Serializer component
11:50:04 <WimLeers> and its $context being a wild west
11:50:19 <WimLeers> and it being utterly unaware of cacheability (granted, that's a Drupal concept)
11:50:31 <WimLeers> Fact is that we need to "shoehorn" in cacheability support
11:50:36 <WimLeers> the only way we can is via $context
11:50:37 <damiankloip> yeah, we have no choice but to use it, as you have no other way to do anything with the serailizer
11:50:47 <damiankloip> yep, totally
11:50:55 <WimLeers> damiankloip: timmillwood I could live with this constant living in NormalizerBase TBH
11:51:00 <WimLeers> there's already so much "not ideal"
11:51:08 <damiankloip> let's be clear, there is no alternatives to $context really :)
11:51:12 <WimLeers> better to encode Drupalisms in our own base class that you already kinda have to use already
11:51:27 <damiankloip> yeah
11:51:56 <damiankloip> timmillwood WimLeers I think the interface is not the worst thing, seems better  to use that in calling code over the base class
11:52:18 <timmillwood> is a constant in serialization.module worse than on NormalizerBase?
11:52:56 <timmillwood> It's not very OOP, but seems to get away from adding interfaces which might come back to bite us.
11:53:15 <WimLeers> timmillwood: yes it is, because anything in .module  is not available in unit tests
11:53:36 <damiankloip> yeah, I would rather just see the interface I think
11:53:43 <timmillwood> WimLeers: urgh, of course!
11:53:58 <damiankloip> it would be nice to have CacheableNormalizerBase too, but I don't think we can do that either
11:54:08 <timmillwood> damiankloip: ok, I'll fix the failing tests later today, and we can move on!
11:54:16 <damiankloip> timmillwood++
11:54:19 <WimLeers> timmillwood: damiankloip: and then NormalizerBase would have to implement CacheableNormalizerInterface instead of NormalizerInterface, which would mean all subclasses get that constant "for free". IT also clearly signals that we in Drupal expect normalizers to do MORE than Symfony normalizers: we expect them to care about cacheability!
11:54:24 <timmillwood> damiankloip: feel free to open a follow up for CacheableNormalizerBase ;)
11:54:37 <WimLeers> timmillwood: no, see the above, we specifically don't want that :P
11:54:46 <WimLeers> that would be a BC nightmare
11:54:50 <timmillwood> ok
11:55:03 <timmillwood> We can close that follow up with "works as designed" then.
11:55:05 <WimLeers> damiankloip: do you agree with what I wrote at :54:19
11:55:10 <damiankloip> WimLeers yeah it's a bC nightmare, not saying we do it. It would just be nice to have Base > cachable base
11:55:14 <damiankloip> this is ideal world stuff
11:55:27 <damiankloip> but that also depends on having a completely new serialization system :P
11:55:58 <WimLeers> damiankloip: in my mind, _all_ normalizers for Drupal must be cacheable. But we don't need to repeat "cacheable" in all normalizer names everywhere. Hence just having NormalizerBase implement CacheableNormalizerInterface seems sufficient
11:56:11 <WimLeers> damiankloip: does that make sense?
11:56:20 <damiankloip> WimLeers yeah, it certainly makes sense
11:56:25 <WimLeers> ok cool
11:56:32 <WimLeers> timmillwood: looks like we have consensus then :)
11:56:34 <damiankloip> in the future who know, in this world, we have to do that
11:56:34 <drupalbot> WimLeers: I'll pass that on when larowlan is around.
11:56:41 <damiankloip> yes timmillwood, go forth
11:56:49 <timmillwood> damiankloip++
11:56:57 <damiankloip> WimLeers++
11:57:04 <damiankloip> timmillwood++ again
12:00:30 <WimLeers> damiankloip++
12:00:32 <WimLeers> timmillwood++
12:00:34 <WimLeers> timmillwood++
12:01:01 <timmillwood> WimLeers++
12:01:11 <timmillwood> You can have karma, even though you introduced the issue :D
12:01:37 <WimLeers> timmillwood: thanks :P
12:01:43 <timmillwood> ;)

Put a breakpoint in \Drupal\Core\Cache\Cache::mergeTags and debug while running the test: that should allow you to figure out why fewer cache tags are being bubbled with this patch.

+++ b/core/modules/serialization/src/Normalizer/NormalizerBase.php
@@ -92,8 +89,8 @@ protected function checkFormat($format = NULL) {
+    if ($data instanceof CacheableNormalizerInterface && isset($context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY])) {
+      $context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY]->addCacheableDependency($data);

Minor, but this could use static:: now, I think.

#17 works for me, I tested it with the fixed_block_content module that depends on HAL+serialization but not on the rest module.

@manuel.adan Interesting, I didn't know that module! Thanks for posting feedback here, and testing the patch!

+++ b/core/modules/serialization/src/Normalizer/CacheableNormalizerInterface.php
@@ -0,0 +1,21 @@
+ * Defines the interface for cacheable normalizers.

s/cacheable normalizers/normalizers producing cacheable results/

This also needed to update the CR: https://www.drupal.org/node/2918937 — did that.

Fixed the docs nit in #19.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!