Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
There are two problems when you for example want to display a poll on a node using an entity reference field:
a) Forced redirects after vote/cancel to poll/ID
b) The output is render cached, resulting in incorrect output for other users.
Proposed resolution
a) Improve the redirect handling, remove completely from vote submit callback, add a destination to the cancel confirmation form, so we get back to the current page.
b) Discussed a bit with WimLeers, the only option we see is to always render the form in a post_render_cache. That allows it to be safely embeded everywhere, with the downside that the form needs to be built on every page and can't be cached (except page cache for anonymous users without a session).
The advantage is that voting doesn't require a cache tag deletion on the poll ID anymore, as this could possibly result in cache clearing everything (if the poll is displayed on every page in a block, for example). It was only a bogus hack anyway, it made the test work but only because that test did the first request after the cache clear.
Remaining tasks
We could consider to improve the performance for anonymous users by caching 3 variants:
- Vote form
- Results with cancel
- Results without cancel
They should always be the same, so we just need to figure out which case we
Another alternative would be to move more logic out of the view form (like the title, and fields (poll with an image..)) into the view builder, possibly actually using the normal view builder and inside that display the form, which would be limited to the actual form elements. Probably even two separate forms for vote and results to make it less complex.
I think both options can be (separate) follow-ups, but I wanted my thoughts down.
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | poll-embed-2335425-14-interdiff.txt | 1.42 KB | Berdir |
| #14 | poll-embed-2335425-14.patch | 12.12 KB | Berdir |
| #11 | poll-embed-2335425-11-interdiff.txt | 1.83 KB | Berdir |
| #11 | poll-embed-2335425-11.patch | 12.11 KB | Berdir |
| #8 | poll-embed-2335425-8.patch | 10.27 KB | Berdir |
Comments
Comment #1
BerdirFirst implementation, seems to work quite fine, used the comment form as an example.
Patch depends on #2331515: Allow to show the poll question in the vote/result templates, which depends on #2331461: Clean up entity handlers/forms, just posting in case someone wants to review this (Hi Wim :)).
Also needs tests.
Comment #3
Wim LeersLooking good :)
Comment #4
BerdirDid some testing with page and render caching. Had to make a few more changes, one thing was the missing validation in case a form submit from a cached page happens for an IP that already voted, another thing is re-adding that the results page is not cached.
I have a project specific behat test, will try to tansform that into a simpletest when the other two issues are done.
Comment #8
BerdirRe-roll, conflicted on the renamed getCacheTags() method that we remove here.
This will fail on some tests but those tests are bogus I think and we need more tests as mentioned above.
Comment #10
kopeboy CreditAttribution: kopeboy commentedWill the poll cache per roles that can't vote?
(I'm not a dev..)
Comment #11
BerdirRerolled and updated the tests to reflect how this is now actually working, which I think is the correct way and only possible way, the tests were cheating.
@kopeboy: That would theoretically be possibly, by checking that in the post render cache callback, and returning a cached version for those users. Also note that the page cache is supported, so anonymous users will get a cached page, and it will be validated on submit if their IP is still allowed to vote, as demonstrated by the test.
Comment #12
adammalonespelling mistake on return.
Is this comment meant to have the semi-colon and #? phpcs calls this up on not starting with a capital letter.
Other than that, I don't see any problem with this, indeed allowing polls to be embedded correctly is desirable.
Comment #13
BerdirThanks. Will fix the reeeturn, the description there is copied from CommentPostRenderCache in core I think. I can rewrite it to Callback for #... or so.
Comment #14
BerdirFixed that, will commit once I confirmed that there was no core change that broke something.
Comment #16
BerdirStill green, committed. Great, that was the last poll customization we had.
Next up: #2409887: Views integration.