Problem/Motivation
Custom modules / themes and other contrib modules like views_field_formatter use the API function of views: views_embed_view()
HEAD
In HEAD views_embed_view() returns the INNER views render array, so it doesn't include any kind of cacheability metadata, so using it can cause cache poisoning and introduce a potential security issue. For example views_embed_view() is used twice, it could show the same result for different views.
PATCH
With the patch views_embed_view() will not return the views render array, but rather the views element, which will be expanded on render time to the actual views render array.
Proposed resolution
Remaining tasks
User interface changes
API changes
The return value of views_embed_view() will still ultimately be a render array, but its structure will change, see https://www.drupal.org/node/2649352#comment-10818662 for details.
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #47 | views_embed_view-2649352-47.patch | 2.63 KB | borisson_ |
| #47 | interdiff.txt | 675 bytes | borisson_ |
| #13 | Screenshot10.png | 75.15 KB | pepe roni |
| #13 | Screenshot9.png | 26.9 KB | pepe roni |
| #13 | Screenshot8.png | 35.21 KB | pepe roni |
Comments
Comment #2
pepe roni commentedadded missing (but important) configuration for article view display.
Comment #3
pepe roni commentedAdditionally this happens to multiple value fields, too, if the formatter is configured to display the view for each occurence.


If it ist configured to concat the values (eg with "+") and display the view only once, it works.
Comment #4
pepe roni commentedBut why do you have to save the node to achieve the changes in the display formatter?
Comment #5
polHi there,
Thanks for the bug report.
What I understand here is:
Could you post some screenshot of your content type configuration, an overview of the fields and of the view ?
I'm unable to work on that today, but it will help me narrow the problem if I have screenshots.
Comment #6
pepe roni commentedHi Pol,
you can start with a vanilla D8 installation and then import the configuration files from the zip file in the attachment of this issue. Then you can play with the data. Thanks to D8 it is now so easy to provide sample configuration ;)
And yes, you are right, I didn't catch that. The delta values are always incorrect. And the values are the same not only for different fields but for different occurrences of the field, too (see post #3). In post #3 the first arguments should vary from 99 to 97(99+98+97 is correct in the second case), the third argument should vary from 0 to 2 (0+1+2 in the second case).
And, don't forget to check this: the changes in the formatting should be applied immediately, not only after saving the node again.
BTW: what about the revision id? Wouldn't it be enough to pass the entity id, the revision id and the field delta to the view? Everything else could be determined by the view itself, especially the field value. Or, like viewfield does, pass the arguments by tokens (e.g. something like [node:name-of-field:value], [node:id], [node:name-of-field:delta])
Comment #7
polI'm unable to import the configuration.
Comment #8
pepe roni commentedThe synchronize tab will still list 6 changed config files as they are missing the uuid.
Comment #9
polI did less or more the same by including system.site.yml in the config archive, then it worked.
I'm actually trying to reproduce the problem, but so far, everything works fine here.
Don't forget that, if you change anything in the view, you'll have to edit the formatter, then SAVE it (very important), so the content can be refreshed.
Comment #10
polI'm sorry but I'm unable to reproduce the problem locally.
Here's a couple of screenshots...
Comment #11
pepe roni commentedI attached my sites-folder (containing a a sqlite database file) to show the problem... (user: me, password: me)
Comment #12
polSorry no support of sqlite on my laptop, can you post screenshots ?
Comment #13
pepe roni commentedIt does not depend on the database system. Checked with sqlite and mysql. Same behaviour. Attached you will find some additional screenshots.
I'm on Kubuntu linux 15.10 with PHP Version 5.6.11-1ubuntu3.1 and Apache/2.4.12 (Ubuntu)
Comment #14
pepe roni commentedI think it's a caching problem. I modified the plugin to show the parameters passed to
views_embed_viewand these were correct. When I add the view to other fields, they display the values of the last call of the view. When I save the node, the cache is refreshed and all field display the same values (of the first call of the view after cache refresh).Played with drupal 8.0.3-dev without any success. Disabled caching in the view, in the views advanced settings and in admin/config/development/performance with no success.
So, what's wrong? Any Idea?
Comment #15
pepe roni commentedI change the subject and project of this issue, as it seems to be a problem with caching in views (or my default settings in php.ini). Checked the problem with different (linux based) servers and all show equal faulty results.
Comment #16
dawehnerI'm wondering what does views field formatter actually do? I'm pretty sure its used in the wrong way
Comment #17
pepe roni commentedMaybe, but that's not the problem. The problem is: with Pol's environment it runs as expected, in my environment it does not. The parameters passed to views_embed_view are correct, but not correctly displayed in the view.
You can optimize the code, of course, and replace views_embed_view by loading the view only once per field and then render it with different parameters through the preview-Method. But this does not solve the problem (same results).
Comment #18
polHi,
Just for the record, here's an explanation of the purpose of Views Field Formatter (VFF):
Right now, the module works properly on my laptop because I've disabled the caching (see [#2598914]).
Indeed, if I enabled caching again, I run into problems.
Comment #19
pepe roni commentedThose settings described in [#2598914] work for me now, too. But what about production settings? I was only testing the configuration of that module and ran into the problems. One can't put this to production! Indeed it seems to be a core problem.
Comment #20
dawehnerSo I was about to propose the following change to
views_field_formatterbut then I realized, we need the same kind of behaviour inviews_embed_viewitself, to be sort of usable.Comment #22
pepe roni commentedThanks, Daniel. The patch for views_field_formatter seems to work. It could be so simple to use a render array instead of the render method (through views_embed_views) instead ;)
Comment #23
polCommitted already and preparing a new stable release :-)
Comment #24
polThe core patch is ok for me but the return format worries me.
The documentation of the function says:
Now that with that patch
views_embed_views()will not return HTML anymore but a render array, should we update the documentation too ?Comment #25
dawehnerIt already didn't do before the patch, it always returned a render array in d8. The only difference is that this one has cacheablity metadata added.
Comment #26
polThen it's perfectly fine for me.
Comment #27
damiankloip commentedThis looks good to me. I am just wondering why we never removed views_embed_view. The usage of it should be discouraged IMO, and we should mark it deprecated. I think we talked about removing it at some point, but this was years ago in Paris I think.
Comment #28
dawehnerI guess we have been just too lazy!
Maybe we should even deprecate it at that point in time?
Comment #29
tim.plunkett+1 for deprecating in a follow-up
Comment #30
alexpottWell let's add @return documentation then ... but oh hum this is an API change. I think this patch should actually just become the deprecation... and tell people to use render arrays instead.
Comment #31
tim.plunkettWe could add
views_embed_view_this_one_actually_works()Comment #32
dawehner@alexpott
I don't agree that we should consider this as change. I mean at some point there will be render caching involved in which case, you'll have security issues.
It is already a not really alterable render array, so I don't think we really consider this as API.
Comment #33
alexpott@dawehner ok... so the security argument is compelling BUT we do have a problem with something that is using it and just joining to other strings to make an email for example. I'm not sure how this plays wrt to BC policy.
Plus do we need to do the access checks in views_embed_view() - don't we need the render array no matter what so that all cache contexts are present?
Comment #34
dawehnerI don't understand your point. Its already a render array, so in order to make something useful, in emails you would need to render it to a string in the first place.
In that case it would not be a different output.
I think we ideally skip rendering but set always a
#access.Comment #35
xjmCan we get an issue summary update? I'm having a lot of trouble evaluating the BC versus the security for this issue and I can't connect the dots between what is in the summary and what I see in the patch. Maybe we do not need all those full-screen screenshots there? And it would be good to document the implications of the BC break and of the potential cache poisoning/cache invalidation problem in the summary too.
For me, my initial reaction to this is that changing the return value structure of
views_embed_view()is extremely disruptive, and would be off the table for a patch release especially. So yes, let's deprecate it it for sure (in another issue), but the fact is that a lot of sites rely on that function. But a clearer explanation in the issue summary would help make recommendations about the BC.Also tagging for a CR, which this will need in any case and which might help clarify what exactly the BC break is. Thanks @dawehner!
P.S. -- I also do not understand @alexpott's first paragraph in #33.
Comment #36
dawehnerYeah I think he believes that
views_embed_view()returns a string or maybe that getting a render array and altering that is a valid way to interact with drupal.Do you really think so? Like how high is the chance that someone actually relies on the exact behaviour. For my point of view, people just care that something is rendered and don't look into a deep level of render array crap.
All of those sites than have issues with security, in case there is any kind of dynamicness or what not.
I won't write a change record, I just don't have the power for that.
Comment #37
tim.plunkettI can see the confusion, every part of the call path either has no @return is or documented to be a string.
The fact is, it returns an array via \Drupal\views\Plugin\views\display\DisplayPluginBase::render()
Comment #38
xjmThanks for the summary update @dawehner.
Well this is why I asked about a change record, because I could be totally misconstruing what the implications are in context of a site/module that is using it. :) There is a reason that render arrays are considered an internal API for sure; modules should not rely of the internal details of their structure and the fact that many things in Drupal must when altering them (because ArrayPI) is something we have to manage the disruption for.
Like a side-by-side comparison of the before and after return value would be good for the CR. Because DisplayPluginBase::preview() looks like a totally different animal. But this also goes back to the whole internal complexity around Views rendering that I have always struggled to grok anyway, so I could be misunderstanding.
Edit: I totally crossposted with @tim.plunkett; reading now.
Comment #39
xjmFor the cache poisoning problem generally, since it depends on the particular usecase, we could consider a middle ground where for 8.0.x we provide a warning or workaround to invalidate the cache somehow -- so no sites blow up, but we reduce the risk of cache poisoning between now and 8.1.0 (which is when the logical time is to change the render array structure if it's only a matter of one render array structure versus a different one, without any lost data or change of actual data type for other callers).
Now that I sort of grok @alexpott's example about emails (thanks @dawehner), I think that is not a concern. Sites/modules that want to email the output of a view or concatenate it with something else can
renderPlain().Comment #40
xjmComment #41
tim.plunkettOpened #2661504: Document return values of Views render pipeline correctly for #37.
Comment #42
xjmI talked about this with @catch some more. We decided to allow this in a patch release with a release note mention because:
We're going to commit this issue (once it has a CR and the proper return docs for the changed method and is RTBC) as soon as possible after 8.0.3. Then the CR will go out and the 2 people on earth who might be affected by the internal disruption have some warning to fix it if needed. Let's also get that followup issue to deprecate this function.
I'm also upgrading this issue to major, because cache poisoning is bad and also because the original bug reported was pretty funky for doing something that seemed pretty normal.
Thanks @dawehner and @tim.plunkett for all your help clarifying what was going on here.
Comment #43
xjmComment #44
tim.plunkettComment #45
alexpott@dawehner the reason I thought this was an API change was...
So we need to fix the
ViewExecutable::render()documentation in another issue. Ah.... looking at @tim.plunkett's comment in #37I think this patch should at least add @return documentation to
views_embed_view(). We're not changing the type of what we're return, as pointed out (and seen though testing) it is a render array, but we are changing the nature return value. For example, the output ofviews_embed_view('frontpage');has changed...From:
To...
Comment #46
dawehnerThank you alex for posting it
Comment #47
borisson_This patch adds the docs from #2670726: views_embed_view() does not document return value.
Comment #48
lokapujyaMissing return argument.
Comment #49
dawehnerThis is alright, I mean this is a NULL.
Comment #50
lokapujyaFine, I'd prefer to see explicit NULL on a non void function (one that also returns values) though, but don't care that much. Basically, because it puts an end to this https://www.drupal.org/node/2670726#comment-10875394.
Comment #51
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
I added issue credit for the initial discussion/STR and the clarifications on disruption.
Comment #54
xjm