πŸ‡ΊπŸ‡ΈUnited States @EclipseGc

Account created on 19 May 2006, almost 18 years ago
#

Recent comments

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Webchick!

I hope you know you'll be incredibly missed. I hope your current work is supremely fulfilling. We'll all miss you. Go make ANOTHER dent in the universe.

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

@FiNeX

We'd need the layout section to support such a thing. Definitely a separate issue from this one. It needs its own acceptance criteria and evaluation of whether or not we want to actually do it.

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Context tokens are 100% a thing we should be eventually chasing down, but not a thing we really do today. They're especially useful for content blocks, but it's definitely a separate feature.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Ok great, so this is not a bug that you've managed to make happen w/o a core patch. That's good news.

Plugins don't require contexts FROM a specific place. They require contexts of a specific type (say, "node" for example). The config saves the expected key from the contexts array that was configured through the UI, so contexts that are available through the admin that cannot be calculated during the runtime of a page are unsafe. There's a fair amount of code elsewhere in core reenforcing that approach. The best thing to do for the patch in question would be to toss a try/catch around the instantiation of the plugin. If we catch, we can return something else and add higher priority event subscriber that logs the error, stops propagation and returns an empty array as the render of the block without invoking the rest of the event subscribers.

I've not looked at the patch in question, however one of the reasons we've not yet gone after "relationship" style contexts is precisely because we have to compensate for this situation in which the context is calculate-able in the administration, but doesn't actually exist during run time. I'd say it's the domain of that patch to fix. But the problem doesn't invalidate the current code path's approach. We know and understand that this is a real thing. We planned for it and have dealt with it similarly elsewhere.

@see:

\Drupal\block\BlockAccessControlHandler::checkAccess lines 92 & 122

It's not a perfect match in that case because we specifically delay context mapping until access checking in the block module approach so we can just return AccessResults in that case. In this case, our new higher priority event subscriber would need to return an empty build array, but it also needs to addCacheableDependency() for an AccessResult::forbidden() on the event itself. It all seems super achievable, but again I'd say it's outside the scope of this issue. However once this issue lands, ✨ Make it possible to add relationships to layout builder Needs work would need to do something similar for visibility plugins to try/catch their context mapping. Still it's 3001188's issue, not this one's.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Neither issue cited above gives insight into how you've managed to achieve this particular bug. Also, I question your premise. Let me explain why:

  • \Drupal\layout_builder\Section::toRenderArray() is responsible for getting the render array of all the blocks and regions representative of a given section.
  • \Drupal\layout_builder\Section::toRenderArray() calls: \Drupal\layout_builder\SectionComponent::toRenderArray()
  • \Drupal\layout_builder\SectionComponent::toRenderArray() dispatches the Event in question.
  • Contexts are already provided to the event object which is capable of getting a block plugin via: \Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent::getPlugin()
  • The event's __construct() method delegates all the instantiation and context setting bits of the block to: \Drupal\layout_builder\SectionComponent::getPlugin()
  • Event subscribers are free to get the plugin or configuration details and make use of it as necessary.
  • \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray actually calls the build() method of the block. This event has a priority of 100.

With that flow in mind, the block plugin, with its context(s) (set properly or otherwise) exists before the render array is attempted to be generated. This fact means there are two potential solutions to the problem you outline above (though again, how you got there is beyond me since the UI strictly prevents the system from getting into this sort of state in the first place).

  1. We can properly nuance the existing event subscribers to try/catch this themselves and return empty build arrays with the appropriate caching logic as necessary.
  2. It might be possible to build an event subscriber which considers all the plugins under the section and checks their contextual status w/o invoking them. This sounds like new-api territory, so again without understanding how you got to this point I suspect this is a step further than is necessary.

If I've misinterpreted your issue, please elaborate further. I do not favor moving away from the EventSubscriber approach. It buys us too much.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Yeah, that's a failing of the css reset on the settings tray. An on-going problem in core. :-( I've reached out to people who would know how best to solve it.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Also, the patches I've provided are against head and no longer dependent on the defaults issue, so apply these patches, and forgo the defaults patch for the time being (until it lands and I reroll).

curl https://www.drupal.org/files/issues/2937799-8.patch | git apply; curl https://www.drupal.org/files/issues/2916876-16.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Yeah, it's a totally annoying thing, but you'll need to follow Tim's directions. :-(

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Ok, fixed the missing end line I had in the last patch and made editing a configured visibility condition actually work now.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Ok, I've rerolled this on top of #2937799: Allow greater flexibility within SectionComponent::toRenderArray() β†’ after abstracting that code out of this patch. Also, this should apply directly to core now with only that 1 patch applied. Layout defaults are a separate thing and I don't think it's affected by this code path at all.

I've not provided an interdiff because I actually wasn't certainly how best to get one given that much of the code I removed from the patch still exists in the code base. Whatever the case, the main patch should be significantly smaller now. Fixed a couple of docblocks I'd missed here and there.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Also, if you're wanting to try out a full demo of what we're pursuing, this is accurate as of now, and all the patches should layer just fine:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git layout_builder
cd layout_builder; curl https://www.drupal.org/files/issues/2922033-layout_defaults-62.patch | git apply; curl https://www.drupal.org/files/issues/2916876-13.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply; composer install

So hopefully that helps with bootstrapping review.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Ok, I didn't have a chance to reroll against only head, so this patch is dependent on #2922033: Use the Layout Builder for EntityViewDisplays β†’ comment 62. I will get a version of this patch for core Jan 18th.

There are a bunch of interesting bits in the interdiff. Most importantly, I rewrote how SectionComponents are rendered at all. This will likely need to be split out into a parent issue of this issue, but for the sake of explaining what's here, I converted SectionComponent render array building into an event which is dispatched. This is super powerful because it means other modules can get involved in the render array building process. It also means that layouts COULD potentially render things which are not blocks. Tim wrote the original code that way and I doubled down on it here. I don't think it's a feature we'll practically use, but we essentially get it for free moving to the event dispatcher. Furthermore, being able to run before or after the normal build process means that introducing something like "visibility" into that paradigm became a simple event subscriber instead of me modifying the monolith that did the render work for us (which is what the last patch did).

Net 0 for the SectionComponent class at the moment. The interdiff removes the condition manager that the first patch added, but adds in the event dispatcher service. Still, that's a practical service when I felt that the condition manager really wasn't.

I'll post more tomorrow. (I'm pretty certain visibility condition editing, not creating, is broken. So that's one of tomorrow's tasks.)

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Will do. I'll reroll this shortly.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Developed against preview versions of the patches we were dependent on. It's also dependent on #2922033: Use the Layout Builder for EntityViewDisplays β†’ , so this won't work w/o that.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

Sorry for the trash in that patch. I'll fix it in the morning.

Eclipse

πŸ‡ΊπŸ‡ΈUnited States EclipseGc

How about something along these lines?

Eclipse

Production build https://api.contrib.social 0.61.6-2-g546bc20