Coming to this while testing #1898474: pager.inc - Convert theme_ functions to Twig.
If a pager section is meant to be shown in the view preview, the view is not shown in the preview UI, and an error is logged in watchdog:
Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Passed variable is not an array or object, using empty array instead") in "core/modules/views/templates/views-view.html.twig" at line 73. in Twig_Template->displayWithErrorHandling() (line 279 of /home/s86294e944c6f7fd/www/core/vendor/twig/twig/lib/Twig/Template.php).
This seems related to the fact that the Twig template is expecting a rendered array, while the view pager plugins are still pre-rendering the HTML.
Steps to reproduce:
- create 3 nodes
- promote them to front page
- change the pager setup of the Frontpage view to display 2 items per page
- save the view
- no output in the preview section
Comment | File | Size | Author |
---|---|---|---|
#44 | 2030293-view_preview-43.patch | 8.25 KB | mondrake |
#44 | interdiff_39-43.txt | 4.5 KB | mondrake |
#39 | 2030293-view_preview-39.patch | 8.68 KB | mondrake |
#39 | interdiff_34-39.txt | 858 bytes | mondrake |
#35 | 2030293-35.png | 81.76 KB | R.Hendel |
Comments
Comment #1
mondrakeThis patch changes output of the pager plugins from rendered HTML to rendered arrays.
Note: even if the pager is shown as a result of this patch, navigation between different pages is not working. An issue exists for that, see #1904922: Views UI Preview - pager navigation is broken
Comment #2
mondrakeRe-testing after #1898474: pager.inc - Convert theme_ functions to Twig got committed.
Comment #3
mondrake#1: 2030293-1.patch queued for re-testing.
Comment #4
R.Hendel CreditAttribution: R.Hendel commentedThe patch works very well.
For testing I installed an new empty drupal 8.x and could reproduce the error very easy by generating 50 items with devel and creating a views using a pager with 10 items at each site.
Before patching:
There are no items displayed in preview:
After patching:
After applying patch and clearing cache same view displayes items im preview as expected. The pager itself is also being rendered (out of screenshot...)
Comment #5
alexpottConsidering a php error is demonstrable we have tests to ensure we don't break this in the future
we can just return the array... no need to set $output :)
Comment #6
mondrakeShouldn't be too hard to put tests in. I'll try to take it on.
Comment #7
mondrake#5
- return array directly - OK
- I also changed the docs of the PagerPluginBase render method - in fact this method is no longer 'rendering' the pager, but just returning a render array
- as to the test, I had a go but can get them to work - the idea being to refresh via AJAX call the view preview and then check if all expected elements are there. But the test doesn't work either with or without the patched plugins.
.. so this patch will fail, please anyone can take it up?
Comment #8
mondrakeurgh
Comment #10
mondrakeMaybe I found a way. Patch soon.
Comment #11
mondrakeOkay, so I found out how to render the entire view preview invoking the renderPreview method of ViewUI.
The test-only patch will show the failure (the Twig_Error_Runtime in summary both for Full and Mini plugins), and the full patch should go green.
(if bot agrees of course)
Comment #12
mondrakeunassigning
Comment #13
mondrake#11: 2030293-11-test_only.patch queued for re-testing.
Comment #14
mondrake#11: 2030293-11-test_only.patch queued for re-testing.
Comment #15
R.Hendel CreditAttribution: R.Hendel commentedFor testing I created a new view at simplytest.me containing a pager .
It displays pager in preview so I think this patch works very well.
Here you can see views preview with pager:
Comment #16
YesCT CreditAttribution: YesCT commentedhas tests, and the code looks good.
no api changes, I think.
Comment #17
Dries CreditAttribution: Dries commentedFor some reason, the test-only patch in #11 didn't actually fail. This suggests that the test may need work. I'm moving this to 'needs work' so we can investigate what is wrong and fix it. We should be close though ... :)
Comment #18
mondrakeYes, the test-only patch cannot reproduce the Twig runtime exception. Needs work.
Comment #19
mondrakeI'm not Twig expert, but it looks like Twig is 'compiling' on first call the templates it renders. If on compile the {pager} element in the template is a render array, then it is no longer possible to pass pre-rendered html in further calls. Vice versa, if on compile the {pager} is a string, then following calls can use either render array or string. That's why test-only in #11 doesn't fail (both plugins use theme() instead of render array).
The patch attached is just a half-way between test-only and full patch in #11 - the theme() call is changed to render array for Full pager, but not the Mini one. So on 'compile' the Full is 'fixing' that {pager} should be a render array, and the Mini pager (still theme()) fails (well should :)).
Maybe review from Twig experts could help, tagging.
Comment #21
mondrakeSetting to 'needs review' for review of test in #19.
Comment #22
R.Hendel CreditAttribution: R.Hendel commentedI cannot judge how serious it is, that automatic test results in one exception.
But like before I tested patch I in the same way I described in #15and found no errors.
So in my opinion the patch works fine.
Here you find screenshot:
Comment #23
dawehnerThis is looking great and is nearly RTBC.
Ensure to tag everything as VDC, if it is related with views.
Here is a small follow up which is unrelated: #2036087: Add public identifier to all render methods in all views plugins in core
Afaik we add the root namespace directly here, so
let's add a public identifier
Comment #24
mondrakeThanks for reviews!
@23, patch attached
- removed
use SimpleXMLElement;
- added
public
identifier tofunction testPreviewWithPagersUI()
- corrected a couple of glitches
I'd leave the addition of public identifier to the render method to the follow-up.
Comment #25
dawehnerNearly :)
This should use "\" as well.
Comment #26
mondrakeComment #27
YesCT CreditAttribution: YesCT commentedjust #25.
Comment #28
dawehnerThanks!
Comment #29
mondrakeI am going to post a new patch with better tests soon.
Comment #30
mondrakeTest changed to use drupalGet and drupalPostAJAX to mimick actual sequence of client-server steps.
So we can stay more high level here, and avoid instantiating a ViewsUI class to call the renderPreview method.
Comment #31
mondrakeComment #32
dawehnerI am wondering whether we could reuse the test code between mini and full pagers a bit.
Comment #33
mondrakeWe certainly could put drupalGet, drupalPostAJAX and the checks up to the number of items on the page in a separate protected function. Then we may drop testPreviewController() as that would be redundant. Patch tomorrow.
Comment #34
mondrake#32 and #33
Still, #19 is the only way I could find to have a test failure demonstrating the failure reported in the issue summary. We need have Twig 'compiling' the template based on a render array before it fails on pre-rendered HTML. That means at least one of the two plugins need to be changed to render array for the other one to fail. So no clear cut test-only patch.
Comment #35
R.Hendel CreditAttribution: R.Hendel commentedI tested patch #34 at simplytest.me and found pager displayed at UI.
So just from the behaviour of views this patch works fine for me:
Comment #36
dawehner+1 for the code
I am wondering whether there is an explicit test somewhere which checks that stuff is just rendered once. This might be hard
As a follow up I would suggest to use $this->themeFunctions which would require a "theme = 'pager' and register_theme = FALSE" in the annotation.
Comment #37
mondrake#34: 2030293-view_preview-34.patch queued for re-testing.
Comment #39
mondrakeRe-roll after #2036087: Add public identifier to all render methods in all views plugins in core + minor code style glitch.
Comment #40
mondrakeWas RTBC before re-roll.
Comment #41
mondrakeCreated #2042941: Full pager plugin: reference the 'pager' theme in the annotations. as a follow-up re. comment in #36
Comment #42
alexpottI think the whole foreach loop here is adding complexity... it is simpler and more explicit to write it like this... (also 11 lines of executable code vs 22 in the original... less to maintain :) )
Add this too...
Comment #43
mondrakeMakes sense...
Comment #44
mondrakePer #42
Comment #46
mondrake#44: 2030293-view_preview-43.patch queued for re-testing.
Comment #47
dawehnerReally good idea!
Comment #48
alexpottCommitted 982452f and pushed to 8.x. Thanks!