Rename template files in Sphinx to use preferred .jinja suffix.
#12364
Conversation
Follow-up to pull request sphinx-doc#11916
jayaddison
added
internals:refactoring
internals:other
priority:low
python
|
Nope, not as easy as reverting a single-commit; I'll revisit this again later. |
…plication and test code" This reverts commit cef1059. Conflicts: sphinx/builders/_epub_base.py sphinx/builders/changes.py sphinx/ext/imgmath.py sphinx/writers/latex.py
… references to '_t'-suffix templates" This reverts commit f0f22fd.
… template detection and rendering" This reverts commit 2a5a9a0. Conflicts: sphinx/ext/imgmath.py sphinx/writers/latex.py
|
Question: will this mess with localization/translation workflows? Various $ grep -r '_t:' sphinx/locale | sort | head -n 10
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/builders/latex/__init__.py:199 sphinx/templates/latex/latex.tex_t:91
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/domains/std/__init__.py:571 sphinx/templates/latex/latex.tex_t:106
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/longtable.tex_t:52
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/longtable.tex_t:63
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/sphinxmessages.sty_t:10
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/sphinxmessages.sty_t:11
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/sphinxmessages.sty_t:12
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/sphinxmessages.sty_t:13
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/sphinxmessages.sty_t:8
sphinx/locale/ar/LC_MESSAGES/sphinx.po:#: sphinx/templates/latex/sphinxmessages.sty_t:9 |
No problem, these comments are automatically added and updated by the gettext extractor (here a script based on babel, which should maybe be updated). They are only there as an indication for the translators and are not used when looking up for translation of messages. |
…te-files # Conflicts: # sphinx/ext/apidoc.py
sphinx/writers/latex.py
Outdated
| @@ -437,7 +437,7 @@ def astext(self) -> str: | |||
| 'body': ''.join(self.body), | |||
| 'indices': self.generate_indices(), | |||
| }) | |||
| return self.render('latex.tex_t', self.elements) | |||
| return self.render('latex.tex', self.elements) | |||
There was a problem hiding this comment.
Thanks for the review @AA-Turner. On reflection I'm slightly concerned by this filename suffix adjustment to the LaTeX writer's render interface.
Maybe it's OK as-is, but I think I could rewrite this to be more backwards compatible (in case anyone has subclassed the writer, for example, or make calls to that render method directly for some reason).
sphinx/writers/latex.py
Outdated
| elif template_name.endswith('.jinja'): | ||
| legacy_template = template[:-len('.jinja')] + '_t' | ||
| if path.exists(legacy_template): | ||
| return renderer.render(legacy_template, variables) |
There was a problem hiding this comment.
This legacy loading path logic that I switched in isn't something I'm 100% keen on - it's non-obvious enough to feel a bit ugly behaviour-wise.
I think I'll add a warning message when this legacy path is followed.
There was a problem hiding this comment.
There's also a search precedence question here; do we search all template paths for the .jinja file and then all paths for _t, or do we (as currently implemented) perform a single pass through the template paths and check for both file variants.
There was a problem hiding this comment.
Basically I'm worrying about a security-type concern here: it's that someone could place a _t (legacy) file in a template path that they control, and that this logic would then load then instead of the expected .jinja file.
However: I'm not sure that that particular concern is well-founded, because if an attacker has write access to one of the template paths already, they could equally just write a .jinja file in there, and that would achieve the same effect.
However I do think a warning message would still add some value as it could help people to encourage legacy templates to migrate.
There was a problem hiding this comment.
Given that I'm considering the security concern a non-issue, I think that the file-load-order precedence is less important here, and it is OK to use a logically-simpler single search pass over the template paths.
…uffix (preferred) before '_t' suffix.
|
If anyone with a security/sysadmin mindset could double-check and/or challenge my thinking in this thread I'd appreciate it: #12364 (comment) Aside from anything related to that, I'm now comfortable that the changes here are ready. |
|
Thanks! A |
|
Thank you! (including for the |
Feature or Bugfix
Purpose
.jinjasuffix for templated files within the Sphinx code repository, because they are evaluated as Jinja templates.Detail
_tsuffix (short for 'template') to use.jinjaas the file suffix instead.Relates
.jinjafiles #11916.