Closed (fixed)
Project:
Lupus Custom Elements Renderer
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Reporter:
Created:
23 Jul 2020 at 08:04 UTC
Updated:
30 Jul 2020 at 10:07 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mostepaniukvmAdded local_tasks_block links to response
Comment #3
mostepaniukvmCorrected bubbleable metadata to not bubble up max-age 0 and keep dynamic page cache work.
Comment #4
fago>And yes, you were right, It didn't work in this case too but I fixed it by completely remove tabs with forbidden access from render array before merge cache metadata, see patch
Not sure this really correct, since it would miss custom cache context / metadata added by special routes with custom access checkers + wrongly cache the results then. Also, it seems wrong that the patch only adds cache metadata from the #primary tasks, not the #secondary ones.
Not sure what the best solution here.
Variant A) It would be nice if we would have something like lazy-builders in the render array, so that we would be able to render missing bits after retrieving the entry from dynamic page cache. Maybe is there a way we could do this here, custom-coded for local tasks?
Variant B) We move local-tasks out of the response to avoid making requests harder to cache and introduce a new route for it. I don't see a large issue with the frontend triggering another page request for fetching & lazy-loading that information, but obviously it's not nice to bootstrap Drupal twice for logged in users for every page request.
What do you think?
Comment #5
fagoDiscussed approach:
- Have \Symfony\Component\HttpFoundation\JsonResponse provided json data
- Implement event subscriber onResponse that runs after dynamic page cache, just add local-task data hard-coded there to make sure it does not influence caching (later on we can make it easy to add mory lazy-generatd data for modules also)
Comment #6
mostepaniukvmPrepared patch as discussed
Comment #7
mostepaniukvmComment #8
mostepaniukvmre-rolled patch again
Comment #9
fagothx, patch looks really good. only a minor remark:
shoudl use camelcasing. also not sure about the wording, why "source" data? it'S just the data of te response, right? is "data" taken? in that case maybe "responseData" ?
also, please do not forget to set ticket to "needs review" when there is a patch to be reviewed.
Setting it / leaving it needs work now for the minor remark.
Comment #10
fagoI tested this successfully as well, so let's get this naming remark done and let's commit then.
Comment #12
mostepaniukvmyes, it make sense, corrected naming and merged
Comment #13
mostepaniukvm