The renderer throws away cache metadata from access result if it is not allowed

There was a [previosuly ignored] phpstan error related to Renderer::getCurrentRenderContext() about the return value not being nullable but the renderer using if (!isset($context)). The result can actually be null, so I removed the ignored error and corrected the type hint.

Verified the tests show the problem based on the tags

Committed/pushed to 11.x, thanks!

Not sure about the exception being thrown for this case in 10.1, seems like it could be thrown for code that currently runs OK, so back to needs review for 10.1.x. Maybe we could not throw the exception but do the other changes in a patch release?

The same exception would be thrown today if access is allowed and the element does not have #printed yet, so I don't think that would happen, or do you have a specific case in mind?

I cherry-picked/rebased on top of 10.1.x and all tests pass locally [except for the ones I normally can't get to pass at the moment] so let's see what the bot says.

TwoD no I don't, it would have to be something weird like where access is always currently false, hiding the bug I guess? But also can't think of anything that would go wrong if we didn't throw the exception in a patch release.

If the exception is currently thrown then this just helps print a neater message right?

What's the policy for that for minor releases?

If the exception is currently thrown then this just helps print a neater message right?

No it's a different code path so there is a (albeit very small) possibility we start throwing the exception where we currently don't.

This is already in 10.2.x, the exception issue is about committing it to a 10.1 patch release or not.

We could also just move it back to fixed and not backport.

My opinion is that if someone manages to trigger the code path which does not throw an exception today they're already in dangerous territory but it is "working" because a safeguard is missing.

As catch pointed out:
They would have to be rendering a structure where one part ends up not being accessible, which is of course common and fine on its own, but there can't be a render context available - as then it would be triggering exceptions when access is allowed to any other part of the same structure. As far as I know we don't expect the renderer to ever run without a render context available, hence the exceptions.

I can't think of a useful situation which would be relying on an exception not being thrown when rendering something completely inaccessible without a rendering context and.
If we don't throw an exception here, what's the alternative? Don't bubble the metadata if we don't find a context?

Woah! 😳

I'd call this a bug. Worst case, this could mean user A with no access could view this first, which would result in a render cache/dynamic page cache entry with cacheability that indicates it's cacheable for many users (with some set of permissions for example) and then user B gets a render cache/dynamic page cache hit and sees the same as user A: no access … while they do have access.

So +1 to committing this to 10.1 for the reason specified by @TwoD in #12 (ohai btw, @TwoD, long time no see!).

It's incredible that this bug went undetected for so long 😳

If we don't throw an exception here, what's the alternative? Don't bubble the metadata if we don't find a context?

We could continue but watchdog_exception() and/or trigger_error() E_USER_WARNING - that would tell people something bad is happening but not lead to 500s on live sites.

It seems pretty low risk to backport this as is but OTOH we have a way to make it even less risky by using watchdog_exception() and/or trigger_error() E_USER_WARNING, so why not do that? Anyone who gets this exception is definitely doing something pretty strange and scary, but it's still not nice to find that out by getting 500 after a patch release.

Not sure what happened with the status change in #14, meant to leave it needs work for the same reason as #15.

Went with just triggering a warning. Should be "loud" enough to be noticed but not crash things.

@Wim, hey! Long time indeed, maybe we'll meet in Lille?

Appears trigger_error was added to MR 4394 per #15.

Yeah I think this looks good now. Committed b8ae2f1 and pushed to 10.1.x. Thanks!

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