Context Navigation

← Previous Ticket
Next Ticket →

Ticket #4939 enhancement closed fixed

Opened 2 years ago

Last modified 2 years ago

twisted.web lacks a built-in templating engine

Reported by:	habnabit	Owned by:	glyph
Priority:	normal	Milestone:	Twisted-11.0
Component:	web	Keywords:
Cc:	jknight, itamar@…	Branch:	branches/web-template-4939
Author:	habnabit	Launchpad Bug:

Description (last modified by glyph) (diff)

There's no templating engine for rendering things like error pages and directory listings.

I'm going to port over nevow.page.Element into twisted.web.template.

The goal for this should be to support the serialization idiom of HTML5, rather than XHTML. The implementation should not have to change much beyond the DOCTYPE declaraitons and xmlns attributes to support that though, as I understand it.

Eventually supporting XHTML (is it XHTML5 now?) should be possible as well since it's a couple of trivial serialization tweaks to the same DOM, but small bugs will be coped with better by an HTML5 parser (a little extra whitespace instead of a PARSE ERROR page in the browser).

Attachments

locale-independent-formatting-with-tests-r3.patch (5.8 KB) - added by djfroofy 2 years ago.

Change History

13

Changed 2 years ago by habnabit

(In [31139]) Removing code, adding tests.

There was a lot of dead, unused, or otherwise terrible code which I've pruned out again. Some things were replaced with better alternatives, which had corresponding unit tests added. Coverage is all-around better, which is between 87% and 92% across all of the new code. Most of the untested code is in repr methods and other such things.

I still need to write docstrings for a bunch of things, including the new tests. Also, docs. I at least understand everything better to be able to write new documentation for the code.

refs #4939

20

in reply to: ↑ 19 Changed 2 years ago by glyph

Replying to lvh:

I think valid XML is valid HTML5, is that right?

Um, not quite.

Right, yeah.

Valid X*HT*ML

OK, yes, I realize that you can't have arbitrary tags in valid HTML these days, they have to be part of the spec. Also: no xmlns attributes, the namespaces are implciit in the serialization.

is pretty close to being valid HTML5. You would need to fix the DOCTYPE, and potentially change some <meta> elements in <head>, though.

I do know about <!DOCTYPE html>, so let's do that. But, what <meta> and <head> elements?

It is true that closed tags like <br /> are valid HTML5 (but the SGML form is preferred).

Yeah, but it's valid, and the serializer can be simple if we just do it the same way in both.

21

Changed 2 years ago by glyph

keywords review removed

Thanks again. I think this is actually getting close. I knew there were coverage issues, but I was pleasantly surprised to discover that only one or two of them were actually functional, almost all of them were just __repr__s or no-ops of some kind. (Does 'coverage' even emit stuff for 'pass'? Maybe I shouldn't have bothered commenting on those, because they should just be docstrings.)

Lots of things need docstrings. Especially everything in the tests; twisted.test.test_flatten doesn't have any docstrings at all. I'm not going to enumerate all of these in a list, because it's kinda tedious and it's easy to see the things that don't have docstrings. A method with a 'pass' is a dead giveaway, especially :).
According to coverage run trial twisted.web.test.test_flatten twisted.web.test.test_stan twisted.web.test.test_template, there are several bits of code that haven't been covered by the added unit tests. I'll enumerate them here as a hopefully helpful laundry list for anyone responding to the review:
1. twisted.web.template - many of these methods do nothing, but if they need to be implemented at all, then surely that null implementation is there to do something. It should be tested.
  1. _ToStan.resolveEntity
  2. _ToStan.skippedEntity
  3. _ToStan.processingInstruction
  4. _ToStan.startElementNS
    1. the if nons in attributes suite - line 144
    2. the negative branch of if nsPrefix is None - line 153
    3. Both AssertionErrors - lines 158 and 162.
  5. startDTD
  6. endDTD
  7. comment - the ignoreComment early-return.
  8. flattenToRequest. No docstring and zero coverage for this function, but its name implies it's pretty darn important.
2. twisted.web._element (You can really tell that this is the new, good code that was developed TDD: 97% coverage.)
  1. _Expose.__call__: the TypeError
  2. _Expose.get: the UnexposedMethodError
3. twisted.web._flatten
  1. FlattenerError._formatRoot - completely uncovered.
  2. FlattenerError.__repr__
  3. FlattenerError.__str__ - but this can just be deleted, as __repr__ will automatically delegate.
  4. UnfilledSlot.__str__
  5. UnsupportedType.__str__
4. twisted.web._stan
  1. directive.__hash__
  2. directive.__cmp__
  3. slot.__iter__
  4. Tag.__iter__.
    1. Also this uses old exception syntax, don't do that.
  5. Tag._clearSpecials
  6. Tag._clone, the first two 'if' branches
  7. UnsetClass.__nonzero__
  8. CDATA.__repr__
  9. Comment.__repr__
5. In the tests, there's some uncovered code as well. This indicates that maybe not all of the test cases were brought over and some of the other coverage problems would go away if it were.
  1. twisted.web.test.test_flatten.TestSerialization._test_cancellingSerialization is never called.
Several names do not conform to the coding standard. no_ns_attrs, for example. Some of these are just copying the names used in nevow, but this is an opportunity to make them compliant. I suggest attrsWithoutNS. And flattenstring should be flattenString. I see you already fixed XMLFile though: thanks!
It looks like we're not going to do the sphinx migration before the release, so the docs need to be translated to lore. I plan to do that by hand unless someone has a great idea for how it might be done more quickly.
docFactory has an unfortunate name. The method is called 'load', the role is referred to as a 'template loader'. The classes are named 'something file' or 'something string'. The class attribte is docFactory; but what is it a factory of, and what do docs have to do with it? I'm thinking that it should be called something like loader = or template = in the class, but I haven't fully decided yet. This doesn't really need to be dealt with, but it would be a bonus if it could be.

I think I'm going to deal with most of this feedback myself, perhaps coordinating with others for parts of it. Hopefully we can get it reviewed by tomorrow.

28

Changed 2 years ago by jerub

(In [31266]) Remove inAttribute arg from deferflatten()

Looking at inAttiribute, it's specifically for dealing with <foo bar="bar"bar"> quoting, i.e. stopping XSS attacks when a slot contains data that needs to be quoted differently in attribute context.

When passing something more complicated than a string or something that calls back with a string, such as a Tag instance: inAttribute is reset and quoting doesn't work in a way that is useful.

Removing inAttribute from the public api makes the code's external interface slightly easier to understand.

35

in reply to: ↑ 34 Changed 2 years ago by mithrandi

Replying to glyph:

Replying to jknight:

Why not just remove inAttribute entirely, and always escape "? Makes the internal API simpler too.

I think the answer has something to do with JavaScript. If you could definitively demonstrate that you can put " in a script tag and have JavaScript think that it's really a '"', then great, let's do that.

JavaScript in XML/XHTML is basically a disaster anyway. You can't have a literal "<", unless you wrap it in a CDATA section, but then HTML parsers can't handle it anymore (and nobody is actually serving XHTML as application/xhtml+xml, so you're always dealing with an HTML parser). CSS has the same problem, although "<" occurs less frequently in CSS. You almost always want to put CSS and JavaScript in external resources.

36

Changed 2 years ago by glyph

keywords review added
owner glyph deleted
status changed from assigned to new

Dear Reviewer,

You will find recent test results here, some excellent narrative documentation for your perusal here, and documentation generated with pydoctor here. For your convenience, please also find a full diff of the changes included in the branch here.

I sincerely hope this all meets with your approval.

Faithfully yours,

-glyph

37

Changed 2 years ago by glyph

Most Magnanimous Reviewer,

Here's an correct link to the pydoctor/API documentation (whoops!), as well as a test coverage report, generated by running "coverage run trial twisted.web.test.test_flatten twisted.web.test.test_stan twisted.web.test.test_template". Please sort by coverage descending (click on the 'coverage' column twice) and note that all the new modules (_stan, template, _element, _flatten) all have 100% coverage. (I removed a little bit of dead code, and thus the buildbot tests have been re-run as well.)

Thank you,

-glyph

38

follow-up: ↓ 39 Changed 2 years ago by jknight

I think the answer has something to do with JavaScript. If you could definitively demonstrate that you can put " in a script tag and have JavaScript think that it's really a '"', then great, let's do that.

You can't. A <script> tag is kind of an implicit CDATA section in HTML (although with special rules of its own). You can't use *any* HTML escapes in it, including <. And literal '<' characters are usually fine to include literally, unless they start with "<script" or "</script". And, "of course", you can't emit an HTML comment in a script state and expect it to actually be treated as commented out...

Double-quotes aren't a *special* escaping problem in the script context.

And don't forget about the rest of the special nodes too, including at least style and textarea...

39

in reply to: ↑ 38 Changed 2 years ago by glyph

Replying to jknight:

I think the answer has something to do with JavaScript. If you could definitively demonstrate that you can put " in a script tag and have JavaScript think that it's really a '"', then great, let's do that.

You can't. A <script> tag is kind of an implicit CDATA section in HTML (although with special rules of its own). You can't use *any* HTML escapes in it, including <. And literal '<' characters are usually fine to include literally, unless they start with "<script" or "</script". And, "of course", you can't emit an HTML comment in a script state and expect it to actually be treated as commented out... Double-quotes aren't a *special* escaping problem in the script context. And don't forget about the rest of the special nodes too, including at least style and textarea...

Ugh. I had happily forgotten most of this stuff, but it comes creeping back...

Maybe XML serialization is a good idea. Really strict XHTML parsing lets you do all of this junk with the normal XML rules, right?

40

Changed 2 years ago by mithrandi

owner set to glyph
keywords review removed

twisted.web._element
1. _Expose.__call__: The docstring refers to Expose instead of _Expose
2. Element.renderer: The name of this method feels really confusing to me, given how many other things are already named renderer. How about renaming it to getRenderer?
twisted.web._flatten
1. flattenString: Single quotes are used everywhere in this function, with one inexplicable exception - the only string constants that contain double quotes are themselves written using double quotes, requiring backslash-escaping.
twisted.web._stan
1. allowSingleton
  1. The HTML spec refers to these elements as "void elements", so I think voidElements (or something to that effect) would be a better name for this list.
  2. I called it a list, but it's actually a tuple; stylistically, I would prefer a real list, but I don't know actually know what the Twisted coding standard is regarding list vs tuple.
  3. command, embed, keygen, source, track, and wbs are missing from the list.
2. slot: The name, children, and default attributes are not documented.
3. Tag
  1. The docstring refers to "Prototype", with a capital "P". I think this was meant to refer to Proto, which no longer exists, so this description needs to be fixed up a bit.
  2. slotData is undocumented.
4. Tag.__call__: The special if not kw path seems superfluous.
I'd just like to say that this branch is SUPER FRIKKIN' AWESOME!!!111oneone

So uh yeah awesome. Everything in my list is pretty straight-forward, so go ahead and merge once everything has been dealt with, unless further discussion seems warranted.

42

follow-up: ↓ 64 Changed 2 years ago by mithrandi

Replying to itamar:

Aren't you still in the middle of a discussion about how the serialization is problematic?

I believe the current serialization is fine; the discussion is basically about a proposed simplification of the serialization, which could still be implemented after this branch lands.

Replying to glyph:

Ugh. I had happily forgotten most of this stuff, but it comes creeping back... Maybe XML serialization is a good idea. Really strict XHTML parsing lets you do all of this junk with the normal XML rules, right?

If you are actually using XHTML, then yeah, all of the normal XML rules apply, and you don't have to worry about all the crazy HTML rules. There are several problems with this, though:

XHTML parsing rules are just XML parsing rules, so no error recovery is allowed; this is sometimes good, but sometimes you don't want an ugly disturbing XML parser error when something goes wrong.
Using XHTML requires you to serve your content with a content-type of application/xhtml+xml, which will completely prevent your page from being displayed in older browsers.
Most(?) browsers do not support incremental rendering (and parsing?) of XHTML content.

As a result, "nobody" actually serves up application/xhtml+xml, they all serve up text/html, and thus HTML parsing rules apply, even if you're cheating and using XML/XHTML processing tools to generate your content.

53

in reply to: ↑ 52 Changed 2 years ago by glyph

Replying to jknight:

I believe the current serialization is fine; the discussion is basically about a proposed simplification of the serialization, which could still be implemented after this branch lands.

Well, my original comment was a simplification. glyph brought up that actually the current serialization basically doesn't work at all for script/style content...which necessitates more complication being added.

I think it's a good point to be aware of, and we should definitely improve this later, but as I recall, correct cross-browser parsing behavior inside arbitrary <script> tags was a complete nightmare. It's one thing to try to avoid XSS and cleanly emit text when putting content into the middle of a page; it's quite another to try to allow taking advantage of the most obscure horrible corners of browser parsing.

As you rightly pointed out, the only right way to do this is to treat any javascript or CSS beyond onclick="foo()" or style="font-size: big" as an external resource, not something quoted inline in your page.

I also agree that we should probably rip out inAttribute completely at some point, it's a purely internal detail for right now, but the current code is nice and well tested and that simplification could easily happen later.

54

Changed 2 years ago by jknight

html5lib has the following list of tags for which the content is implicit raw data, and thus shouldn't be escaped (html5lib.constants.rcdataElements): 'style', 'script', 'xmp', 'iframe', 'noembed', 'noframes', 'noscript'.

The simplest correct thing would be to just not quote them on output. And then also explain to people that they need to quote the content of those tags on *input* because twisted.web.templates uses XML not HTML. And then also just tell users to not use "</$tagname" in those contexts, because that just can't be serialized properly.

title and textarea are slightly different in that escapes are accepted, but even if you don't escape, it's still part of the text, not separate elements. It's impossible to include other elements inside one of those contexts. That is, in: <textarea><h1></textarea>

The "comment" is not a comment, it is part of the text. The <h1> "element" is not an element, it's also part of the text. But it's fine to continue quoting text on output for those two.

55

follow-up: ↓ 57 Changed 2 years ago by jknight

As you rightly pointed out, the only right way to do this is to treat any javascript or CSS beyond onclick="foo()" or style="font-size: big" as an external resource, not something quoted inline in your page.

That's not true at all: it's trivial to correctly include complex JS or CSS inline in your page, as long as you know whether you're outputting HTML or XHTML. The nightmare scenario occurred in Nevow because it was (or maybe just the poor users were) never sure if it was reading and outputting html or xhtml, and so you had to try to be compatible with both in the same document...

56

Changed 2 years ago by glyph

Replying to jknight:

html5lib has the following list of tags for which the content is implicit raw data, and thus shouldn't be escaped (html5lib.constants.rcdataElements): 'style', 'script', 'xmp', 'iframe', 'noembed', 'noframes', 'noscript'.

Good to know. None of these tags seems like the kind of thing you should be putting user input into, so I think we're okay :).

We should have a separate ticket for making these work right though, insofar as it's possible.

The simplest correct thing would be to just not quote them on output. And then also explain to people that they need to quote the content of those tags on *input* because twisted.web.templates uses XML not HTML. And then also just tell users to not use "</$tagname" in those contexts, because that just can't be serialized properly.

Yes, this is my recollection as well. There are some strings that you just can't convince an HTML parser to preserve in any sane way, so you have to raise exceptions during rendering if they're present.

title and textarea are slightly different in that escapes are accepted, but even if you don't escape, it's still part of the text, not separate elements. It's impossible to include other elements inside one of those contexts. That is, in: <textarea><h1></textarea>

... but <textarea> < </textarea> will still be seen as a text area with "<" in it, so our current algorithm is in good shape, yes?

The "comment" is not a comment, it is part of the text. The <h1> "element" is not an element, it's also part of the text. But it's fine to continue quoting text on output for those two.

Thanks for the input on this.

57

in reply to: ↑ 55 Changed 2 years ago by glyph

Replying to jknight:

As you rightly pointed out, the only right way to do this is to treat any javascript or CSS beyond onclick="foo()" or style="font-size: big" as an external resource, not something quoted inline in your page.

That's not true at all: it's trivial to correctly include complex JS or CSS inline in your page, as long as you know whether you're outputting HTML or XHTML. The nightmare scenario occurred in Nevow because it was (or maybe just the poor users were) never sure if it was reading and outputting html or xhtml, and so you had to try to be compatible with both in the same document...

If you're outputting XHTML, it can trivially be made to work.

If you're outputting HTML though... I'm not so sure. I can't remember the specific issues, but remember that "HTML" implicitly includes IE6, which means that all the rules are sometimes randomly thrown in the trash. I am fairly certain that there are byte sequences which do seriously different things on IE6 and IE7 as compared to every other browser, and some older versions of firefox used to trip over themselves as well (although nobody really uses an old version of firefox, so we can stop caring about that).

I'm sure that anything even halfway modern will work with the HTML quoting rules you've described, though. And my intention would be to add the "are you serializing HTML or XML, let's be completely clear" API to deal with this issue.

I think I should probably add something to the 'doctype' section of the narrative docs that makes it clear that <script> and <style> tags aren't fully supported yet.

58

Changed 2 years ago by mithrandi

Replying to jknight:

html5lib has the following list of tags for which the content is implicit raw data, and thus shouldn't be escaped (html5lib.constants.rcdataElements): 'style', 'script', 'xmp', 'iframe', 'noembed', 'noframes', 'noscript'.

Blegh. According to the WHATWG spec, <style> and <script> (the other elements don't exist) are "raw text", not RCDATA. But maybe this is just bad naming in the html5lib code? <title> and <textarea> *are* RCDATA, but aren't mentioned in your list.

(The difference is that RCDATA can have character references, but no ambiguous ampersands; raw text cannot have character references, and thus ambiguous ampersands are allowed.)

60

follow-up: ↓ 61 Changed 2 years ago by glyph

jknight: I think we can table the issue, but it would not be good to let users fall into this booby trap completely unawares.

I added a note to the section on formats and doctypes explaining the limitations of not caring whether you're emitting HTML or XML. If you take the appropriate care to set up everything so that it's XML, it currently works; we should have a bug to make it work in XML. In the meanwhile though, users should take care to avoid doing templating in those types of tags.

61

in reply to: ↑ 60 ; follow-up: ↓ 63 Changed 2 years ago by jknight

Replying to glyph:

jknight: I think we can table the issue, but it would not be good to let users fall into this booby trap completely unawares.

Okay.

I added a note [...]

Your note makes it sound like there's only a problem if you include dynamic content in it, but really you can't output a script at all, even if it's static in the template file. I also think that XHTML output should never be recommended for anyone to use, because it doesn't work well.

62

Changed 2 years ago by glyph

status changed from new to closed
resolution set to fixed

(In [31357]) Merge web-template-4939 - incorporate the good parts of Nevow into Twisted.

Author: habnabit, glyph, lewq, jesstess, jerub, exarkun

Reviewer: glyph, idnar

Fixes: #4939

This change pulls in exarkun's latest and greatest page rendering model (hence his well-deserved inclusion on the authors list, even though he didn't make commits to this particular branch) from nevow.page.Element, with several modifications to simplify it and reduce the amount of code in it. Also included is a new narrative tutorial document explaining how to use basic templating, and also that you DO NOT NEED THIS to generate HTML in a Twisted program; you just might want it because it's easy and fun.

This should allow us to much more gracefully and correctly generate HTML within Twisted in the few places we already do, and perhaps also allow us to more easily provide new browser-based, simple management tools for Twisted servers at some point in the future.

63

in reply to: ↑ 61 Changed 2 years ago by glyph

Replying to jknight:

Replying to glyph:

jknight: I think we can table the issue, but it would not be good to let users fall into this booby trap completely unawares.

Okay.

I opened #4972 for fixing this for real somehow.

I added a note [...]

Your note makes it sound like there's only a problem if you include dynamic content in it, but really you can't output a script at all, even if it's static in the template file. I also think that XHTML output should never be recommended for anyone to use, because it doesn't work well.

I think my note is clear in the context that it's presented, but things like this could always be clearer. If you want to open a doc bug for a better explanation before #4972 gets addressed, please go ahead.

Note: See TracTickets for help on using tickets.

Download in other formats:

Context Navigation

Ticket #4939 enhancement closed fixed

twisted.web lacks a built-in templating engine

Description (last modified by glyph) (diff)

Attachments

Change History

1

Changed 2 years ago by DefaultCC Plugin

2

Changed 2 years ago by habnabit

3

Changed 2 years ago by habnabit

4

Changed 2 years ago by habnabit

5

Changed 2 years ago by itamar

6

Changed 2 years ago by jknight

7

Changed 2 years ago by habnabit

8

Changed 2 years ago by habnabit

9

Changed 2 years ago by habnabit

10

Changed 2 years ago by habnabit

11

Changed 2 years ago by habnabit

Changed 2 years ago by djfroofy

12

Changed 2 years ago by djfroofy

13

Changed 2 years ago by habnabit

14

Changed 2 years ago by habnabit

15

Changed 2 years ago by habnabit

16

Changed 2 years ago by habnabit

17

Changed 2 years ago by glyph

18

Changed 2 years ago by glyph

19

follow-up: ↓ 20 Changed 2 years ago by lvh

20

in reply to: ↑ 19 Changed 2 years ago by glyph

21

Changed 2 years ago by glyph

22

Changed 2 years ago by habnabit

23

Changed 2 years ago by habnabit

24

Changed 2 years ago by mithrandi

25

Changed 2 years ago by glyph

26

Changed 2 years ago by glyph

27

Changed 2 years ago by jerub

28

Changed 2 years ago by jerub

29

Changed 2 years ago by jerub

30

Changed 2 years ago by jerub

31

follow-up: ↓ 34 Changed 2 years ago by jknight

32

Changed 2 years ago by jerub

33

Changed 2 years ago by jerub

34

in reply to: ↑ 31 ; follow-up: ↓ 35 Changed 2 years ago by glyph

35

in reply to: ↑ 34 Changed 2 years ago by mithrandi

36

Changed 2 years ago by glyph

37