Comments

mostepaniukvm created an issue. See original summary.

mostepaniukvm’s picture

Status: Active » Needs review
StatusFileSize
new2.27 KB

Added local_tasks_block links to response

mostepaniukvm’s picture

Corrected bubbleable metadata to not bubble up max-age 0 and keep dynamic page cache work.

fago’s picture

Status: Needs review » Needs work

>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?

fago’s picture

Discussed 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)

mostepaniukvm’s picture

Prepared patch as discussed

mostepaniukvm’s picture

mostepaniukvm’s picture

StatusFileSize
new7.63 KB

re-rolled patch again

fago’s picture

thx, patch looks really good. only a minor remark:

+   * @var array
+   */
+  protected $source_data;

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.

fago’s picture

I tested this successfully as well, so let's get this naming remark done and let's commit then.

  • mostepaniukvm committed bc95909 on 8.x-1.x
    Issue #3160956 by mostepaniukvm: Add local-tasks to response
    
mostepaniukvm’s picture

Status: Needs work » Fixed

yes, it make sense, corrected naming and merged

mostepaniukvm’s picture

Status: Fixed » Closed (fixed)