Opened 4 years ago

Closed 4 years ago

#3092 closed change (rejected)

[CMS] Support use of the {% extends ... %} tag in Jinja2 pages

Reported by: kzar Assignee: kzar
Priority: Unknown Milestone:
Module: Unknown Keywords:
Cc: sebastian, trev, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29328184/

Description (last modified by kzar)

Background

Currently our CMS renders the HTML for a page and its template separately. It puts the rendered page into the body variable and the head section into the head variable for the template to use.

This makes sense for the raw and Markdown page formats, but is somewhat limited for Jinja2 pages. For example it would sometimes be useful to define several blocks in the template and then overwrite their contents from various pages. Currently we can't do this, and end up having a bunch of messy conditional logic in the template instead.

What to change

Currently the relevant converter is called for the page based on its format, and then the template converter is called for the template. The rendered page is passed to the template converter as the body and head variables.

When the page's format is Jinja2 and the page contains the {% extends ... %} tag, Jinja2 should be left to its own devices. For those pages template inheritance will be taken care of by Jinja2 and the head section will not be automatically put into the head variable.

Notes:

  • Most importantly this should not break existing pages.
  • Templates will need to be updated to support both forms of template inheritance: {% block body %}{{ body|safe }}{% endblock %}.
  • The documentation will need to be updated to explain this change.

Change History (7)

comment:1 Changed 4 years ago by sebastian

I'm curious how that works. I'd expect this change to break the converter architecture with rather ugly special cases for jinja2. Also it will most likely require some additional boilerplate for pages using jinja2 templates, as you would need to explicitly inherit from the "template" and define blocks, most likely also breaking compatibility with existing websites. If I'm wrong, I'm happy to see a proof of concept.

comment:2 Changed 4 years ago by kzar

  • Description modified (diff)

comment:3 Changed 4 years ago by kzar

  • Description modified (diff)
  • Review URL(s) modified (diff)
  • Summary changed from [CMS] Combine page state with template state for Jinja2 pages to [CMS] Support use of the {% extends ... %} tag in Jinja2 pages

comment:4 Changed 4 years ago by kzar

  • Status changed from new to reviewing

comment:5 Changed 4 years ago by kzar

  • Description modified (diff)

comment:6 follow-up: Changed 4 years ago by trev

When the page's format is Jinja2 and the page contains the {% extends ... %} tag, Jinja2 should be left to its own devices.

I'm not convinced. If you don't want the usual boilerplate, why not just define template=minimal explicitly? Currently, you assume that templates extending other templates will have all the boilerplate included - IMHO that's a rather limiting assumption and produces unexpected implicit behavior. Template inheritance is a very mighty tool, it could for example be used to let the various jobs pages inherit from a common template (we implemented a different solution in reality merely because we didn't want to overuse Jinja).

IMHO, this proposal should be rejected.

Last edited 4 years ago by trev (previous) (diff)

comment:7 in reply to: ↑ 6 Changed 4 years ago by kzar

  • Resolution set to rejected
  • Status changed from reviewing to closed

OK, fair enough.

Note: See TracTickets for help on using tickets.