[meta] ContextProviders must return a Context object for cacheability metadata, even when a value is not available, breaking expectations of calling code

I'm curious, don't we need to add the cacheability metadata always?

@dawehner is right, you'd still need the route cache context. Because the value returned by getRuntimeContexts() still depends on the route that was matched. Since that return value (and the cacheability of it) affect Condition plugins, which are used by block visibility, which is central in block access checking, and therefore hugely important in caching of blocks (which cached variant of a block to retrieve, and which cached partial page to retrieve from Dynamic Page Cache), cacheability metadata even for the case of "nope, no node" at /user/1 (or /foobar for that matter) is essential.

You've asked basically the same question at #2375695-69: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed.4, in June 2015. @Berdir answered it in #70.4. This patch is forgetting the discussion that was had there, and is effectively reverting that change (which was a critical bugfix).

NodeRouteContext is indeed returning a Context object, but it's not erroneous, it's correct AFAICT: it's necessary to ensure that everything consuming those contexts (i.e. using them as inputs) can be cached correctly. The value that is associated with that Context object is NULL. This is also why ContextInterface::hasContextValue() was added in #2513244: ContextHandler incorrectly checks required/optional contexts of plugins, which was a blocker for #2375695.

Is it possible you forgot about that? 😳 I feel like I'm missing something…

Please act more constructively. Simply postponing this to 9.x is not going to solve this.

Especially because Drupal 9 is not allowed to break backwards compatibility — we can only drop deprecated APIs. So we should already look into any potential deprecations here.

#2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed was a critical bugfix to ensure that things using the context system would still be cacheable. #8 is saying as far as I can tell that we should give up on cacheability to get back the DX you imagined. (Today's if ($result['node']->hasContextValue()) {…} vs if (!empty($result)) {…} which you prefer.) But in either case, the calling code needs to know how to interpret the result of getRuntimeContexts(). So I'm not sure the difference is that big?

An initial idea/suggestion: what if we'd add a class NoContext implements ContextInterface {…}? Then NodeRouteContext could return new NoContext(['route']). That'd make the NodeRouteContext:getRuntimeContexts() code clearer. It'd still require similar calling code though — which makes sense: we can't break BC.

In this case, there's no need to store the "negative" data that core needs.

This is not true. We still need to know that this "no context" result varies by the route cache context, i.e. that if a different route were requested, that it may not be a "no context" result, but in fact there may be a context!

Unfortunately, even though I appreciate your suggested solution Wim, I don't think it's workable.

That's totally possible 🙂 It was just an initial suggestion to get the discussion going.

I'm super happy to discuss viable next steps if you still feel like there might be one, but in my mind I just need to do the long-hard work of replacing our core block placement tools with viable solutions that people want to adopt so that we can deprecate the entire block module's way of block placement.

Well, that is going to have to happen in a way that does not break BC. So postponing this to D9 is not actually going to help, because all that D9 is going to be allowed to do is remove things that are @deprecated. See https://dri.es/making-drupal-upgrades-easy-forever.

In other words: we'll have to start doing the work of a better system during the Drupal 8 cycle anyway. So there's no point in waiting :)

This is definitely (VERY!) hard for Drupal core developers, but it makes things much easier for the end users of Drupal of course. I've been doing this for the REST module for the past ~1.5 year now, and in that time, we've accumulated >2000 LoC that will be deleted in Drupal 9. I last updated that "remove BC layers" patch a few days ago: #2893804-6: Remove rest.module BC layers.

In the mean time, I'll simply manually remove this context from block placement tools with better insight into their actual present contexts.

I'm sorry, I'm not familiar enough with those tools and these APIs to understand this 100%. Does this mean you are you going to start relying on the API that #2513244: ContextHandler incorrectly checks required/optional contexts of plugins introduced, or that you're going to work around it by limiting what the user is allowed to do?

So, why not use this issue to do the analysis of what's broken in the current system, and come up with a step-by-step plan that would not break BC that would get us to a place where everything is working? We would be able to mark existing APIs @deprecated, and they'd disappear in 9.0.0.
It sounds like it'll be impossible to have everything work as nicely and simply as we'd want, but that is the cost we have to pay for not breaking BC. But we can make plugins opt in to improved APIs one-by-one, and then offer a better user experience for those, perhaps. That'd be an incentive to update to the new APIs. Plus, there's the trigger_error('Some message saying why you should not be using this anymore but the other thing instead', E_USER_DEPRECATED), which will show up in A) test runs, B) people's actual dev environments, which is another incentive.

In any case, there's nothing stopping us from looking into what the problems are and articulating a plan to fix it for 9. I currently don't fully understand what the problems are. You're the main Context component maintainer AFAIK, so you have the most complete insight.

P.S.: why isn't there a Context component in MAINTAINERS.txt or on d.o?

When creating an image field for a simple contentype (node), create a new user role, non admin, assign the proper permissions to the role so it can create/edit/delete, when creating a new node as this new role, the issue is also present:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getType() on null in

The Alt text is not available for non-admins, see screenshot for reference, so it triggers the error described.

Please ignore my last comment since it was a bad implementation of a hook causing the issue.

Just workaround like #2921859: "Call to a member function getType() on null" when the NodeType condition plugin gets an empty context but with route cache context

Isn't that the same patch from #2?

Anyway, retitling.