isRenderedInCurrentRequest for ViewsBlockDisplay always return false.
A comment in this function:
There is no way to know if a block is embedded on a page, because blocks can be rendered in isolation (see big_pipe, esi, ...). To be sure we're not disclosing information we're not sure about, we always return false.
I think we can actually return a correct value by using BlockAccessControlHandler::checkAccess
I dont think any information disclosure is introduced, as this uses the same access checks as every other block.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2855758-4--views_block_display_rendered_check.patch | 1.09 KB | drunken monkey |
Comments
Comment #2
strykaizerComment #3
borisson_I think we might have to look at writing a test for this? Functionally this works though, I sat next to @StryKaizer as he demonstrated it. I also already did my code style nitpicks before the patch was posted. So that's good as well.
In any case, I really like the simplicity here as well as the added functionality. Looks great!
@Thomas: If you think we don't need tests for this, I feel confident in setting this to RTBC. If you want tests, I'll try to have a look at that tomorrow at #dcnlights.
Comment #4
drunken monkeyOh no, Joris, your nitpick-fu is getting weaker!
- Changed to use dependency injection.
- Broke overlong line across multiple lines.
Otherwise, though, this really looks great. Very simple implementation for something we thought we had "no way" of doing, adds nice functionality – very good.
What I'm worried a bit about, though, is performance. This will potentially load several additional entities on every page request where something (probably mostly facets, especially at this point) wants to know whether a block is displayed. Also, I think there is no caching involved? (Except on the entity load level, I guess – which would cover the most part, of course.) Or does at least the facet source do caching?
I've not checked whether this actually works correctly, but if you two are certain of it, that's good enough for me. Tests would be nice, of course, but I'd commit this without them, too. I'd just like to hear your opinion about the performance impact.
(Not sure which component is right here – "Views Integration" or "Plugins". Let's go with the former.)
Comment #5
strykaizerPerformancewise, if a block search api view exists which has facets (or search api sorts), then yes, this would introduce an entityload per block using the same search api display.
Comment #6
borisson_I don't think the performance hit here is big enough compared to the increase in functionality. I think we can just commit this.
Comment #8
drunken monkeyOK, good to hear!
Committed.
Thanks again, both of you!