Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1963 (closed defect: fixed)

Opened 6 years ago

Last modified 5 years ago

Pages of quickstarted TG 1.1 app do not validate

Reported by: chrisz Owned by: Chris Arndt
Priority: high Milestone: 1.1
Component: TurboGears Version: 1.1 HEAD
Severity: major Keywords: needs documentation
Cc:

Description

The pages of a quickstarted TG 1.1 app have a doctype of xhtml-strict, but they contain unclosed tags, i.e. the pages are not valid. Strangely, even if I set genshi.default_doctype = "xhtml-strict", the pages still have unclosed tags.

Attachments

genshi-default-format.diff Download (678 bytes) - added by Chris Arndt 6 years ago.
buffet_patch.diff Download (4.9 KB) - added by chrisz 5 years ago.
Support for options in render method.

Change History

comment:1 Changed 6 years ago by chrisz

Oops, genshi.default_doctype was the wrong option. When I set genshi.outputformat="xhtml", it works and the pages become valid.

So we should either set genshi.outputformat="xhtml" by default in app.cfg in order to have valid xhtml pages, or change the doctypes to html, in order to have valid html pages (as in TG 1.0).

Second, the app.cfg has an option genshi.default_format which seems to be wrong, it should be genshi.outputformat.

Third, what use has the genshi.default_doctype? It sounds like it adds a doctype if none is found in the template, but I tested this and it does not seem to work.

comment:2 Changed 6 years ago by Chris Arndt

XHTML seems to cause problems with JSLink/CSSLink widgets (see #1956).

genshi.default_doctype is passed directly to the Genshi Buffet template (genshi.template.plugin.MarkupTemplateEnginePlugin) and evaluated in its __init__ method. If it does not work, this seems to be a bug in Genshi.

genshi.default_format is also evaluated by the same method but apparently is not passed by turbogears.view.load_engines. This seems to be a bug in the latter function.

Changed 6 years ago by Chris Arndt

comment:3 Changed 6 years ago by Chris Arndt

  • Owner changed from anonymous to chrisz

If nobody objects, I'll apply the patch for fixing view.load_engines before 1.1 beta.

comment:4 Changed 6 years ago by Chris Arndt

  • Milestone changed from 1.1 to 1.1 beta 1

comment:5 Changed 6 years ago by Chris Arndt

Setting genshi.default_doctype works for me in that it changes the DTD to the one indicated even if it does not match the output format. The default is set to None in view.base.load_engines. I think this should be "html" instead. And there should be a comment in the quickstart app.cfg that this setting has to match genshi.outputformat / genshi.default_format.

The problem with genshi.default_format is (even with the patch for load_engines applied) is that view.base.render always determines the output format for all template engines from the *.outputformat setting, thus overriding any default in genshi.default_format. I see three solutions:

  1. Simply change genshi.default_format to genshi.outputformat in the quickstart app.cfg. Problem with this is that the default_format option will not be recognized and this may confuse people who have read the Genshi documentation and may lead to further bug reports.
  1. Do not set the format to html if *.outputformat is not set and let the template engine plug-in determine the default format to use. But this would mean that we can not always determine the content type from the output format anymore in view.base.render. We then would need to implement the second solution from #1971.
  1. We change the following lines in view.base.render:
    if not format:
        format = enginename == 'json' and 'json' or config.get(
            "%s.outputformat" % enginename, 'html')

to:

    if not format:
        format = enginename == 'json' and 'json' or config.get(
            "%s.outputformat" % enginename, 
            config.get("%s.default_format" % enginename, 'html'))

but this doesn't seem like a very generic solution to me.

My suggestion is that we implement solution 3) for "1.1 beta 1" and examine solution 2) for 1.1 final.

comment:6 Changed 6 years ago by Chris Arndt

There is another problem with XHTML output. There is still a meta tag in the output which specifies a content type of text/html. This should be application/xhtml-xml.

comment:7 Changed 6 years ago by Chris Arndt

Implemented preliminary solution 3) in r5339.

comment:8 Changed 6 years ago by Chris Arndt

  • Keywords needs review added

I think we should completely remove the content type meta tags from the page templates in the quickstart templates. turbogears.controllers._process_output sets the "Content-Type" header already correctly (with content type and charset), there is no need for the meta tags and they will be incorrect, as explained above, when the output format is XHTML.

comment:9 Changed 6 years ago by chrisz

Note that the meta content-type tag has a py:replace="" so it is only relevant when you display the template itself as a file. Contrary to Kid, Genshi does not inject such a tag when the template is rendered. Also, while application/xhtml-xml is the right type, text/html is also allowed for XTML 1 and works better for old browsers if you put it in the meta tag.

So I think the current templates are ok. The only thing we may want to reconsider is whether we really want XHTML to be the default format, not HTML as it was in TG 1.0. At least we should explicitly mention this change in the migration docs.

comment:10 Changed 6 years ago by Chris Arndt

Since r5339 'html' is the default.

comment:11 Changed 6 years ago by Chris Arndt

Actually, genshi.default_doctype still defaulted to None. I changed this to "html" in r5423 so that it matches the default template output format "html".

comment:12 Changed 6 years ago by Chris Arndt

  • Keywords tests added; review removed

comment:13 Changed 6 years ago by chrisz

Since we switched back to HTML, it's now irrelevant, but just as an aside to the comment about the meta tag,  this page explains why it is bad to set the content type to application/xhtml+xml in the meta tag even if you're using XHTML, you should use text/html nevertheless.

comment:14 Changed 5 years ago by faide

  • Status changed from new to closed
  • Resolution set to wontfix

closing as wontfix...

comment:15 Changed 5 years ago by Chris Arndt

  • Status changed from closed to reopened
  • Resolution wontfix deleted
  • Milestone changed from 1.1b2 to 1.1

It seems like setting genshi.default_doctype to default to html wasn't such a good idea. The Genshi buffet plugin will then add a doctype to pure XML output, even if format=xml is specified in the expose decorator. This, for example breaks the Genshi output of the new TurboFeeds version 0.2b where the doctype is inserted even after the XML declaration at the top!

Is there any way to work around this? Is this a bug in the Genshi plugin or the expected behaviour?

What would be really helpful if TG allowed to set template engine options on a controller (method) basis and not only globally but unfortunately the buffet API is not designed for this.

comment:16 Changed 5 years ago by chrisz

I mentioned this problem already  here. We would need to patch the Genshi buffet interface to really solve this properly. I'm still using Kid in TG 1.x, or Genshi directly in TG 2.x, so it did not hurt me and I never took the time to work on this, but I still think it must be solved so that Genshi can be a real alternative to Kid in TG 1.1.

comment:17 Changed 5 years ago by Chris Arndt

I see the Genshi folks offered to include a patch, if we provide one. That would take time, though. Bummer. I don't really like releasing TG 1.1 final with this bug. Maybe we should include our own Genshi render engine? But that would mean we'd have to use another plugin name so that wouldn't work well. The only option I currently see is to monkey patch genshi.template.plugin.MarkupTemplateEnginePlugin.

comment:18 follow-up: ↓ 19 Changed 5 years ago by Chris Arndt

I also just discovered that there already seems to be some support for passing extra options to template engines in the expose decorator: the mapping parameter is passed unaltered down to view.render via controllers._execute_func and controllers._process_output, but there it is silently discarded by the adapt_call function if the template engine's render method does not support it.

Regarding providing our own Genshi renderer, it occurred to me that we could just cheat in view.load_engines and replace the genshi plugin class with our own implementation (maybe making this configurable) when loading the template plugins.

What do you think?

comment:19 in reply to: ↑ 18 Changed 5 years ago by chrisz

Replying to Chris Arndt:

Regarding providing our own Genshi renderer, it occurred to me that we could just cheat in view.load_engines and replace the genshi plugin class with our own implementation (maybe making this configurable) when loading the template plugins.

Good idea. We could replace Genshi's MarkupTemplateEnginePlugin (the Buffet interface) with our own version that provides what we need - namely the ability to render different doctypes for different formats. We don't need to document it or make it configurable; it should be a temporary internal bugfix/workaround only. When we're satisfied with our improved Genshi Buffet interface, we can then ask the Genshi folks to take it over to Genshi. Therefore we should try to be backward compatible to the old interface and make only the necessary changes.

I agree we should try to implement this (internally) in TG 1.1 already.

comment:20 Changed 5 years ago by Chris Arndt

Ok, I will try to come up with an implementation tomorrow or the day after.

What do you suggest how we determine the doctype to use? Just use a fixed mapping from format to doctype? Should we just try to fix incompatible doctype/format combinations?

comment:21 follow-up: ↓ 22 Changed 5 years ago by chrisz

There are actually two separate issues:

a) Having appropriate default doctypes for different formats (must have)

b) Additionally, allow full flexibility to have a doctype different from the default for a particular controller (nice to have)

Regarding a)

We could allow setting default_encoding as a mapping from format names to doctype names. The default value for this mapping could be simply

'html': 'html', 'xhtml': 'xhtml'

Then the code in _get_render_options could be changed to:

    if self.default_doctype and not fragment:
        if isinstance(self.default_doctype, dict):
            if format in self.default_doctype:
                kwargs['doctype'] = self.default_doctype[format]
        else:
            kwargs['doctype'] = self.default_doctype

Regarding b)

three ideas how this could be solved:

1) We could add an optional option keyword parameter with default None to the Buffet render() method and then do something like:

    if options:
        kwargs.update(options)

This would allow overriding any option, including the doctype, at render time. Unfortunately, though I don't understand why, the standard Buffet interface does not include any such optional parameters. So we would not comply 100% with the standard.

2) We could somehow pass the docstring along with the format, e.g. separated by a blank or as a tuple. This is similar to how TurboKid passes the Kid-specific output format parameter.

3) We could pass additional parameters along with the template name in URL query parameter notation, as breve does (see  here).

Or maybe all of this is not needed, since as a workaround you can pass an instantiated serializer, like this:

from genshi.output import XHTMLSerializer
format = XHTMLSerializer(doctype='xhtml-transitional')

If this is an acceptable workaround for b) (haven't tested it yet), we just need to document it properly.

comment:22 in reply to: ↑ 21 Changed 5 years ago by Chris Arndt

  • Status changed from reopened to new
  • Owner changed from chrisz to Chris Arndt

Replying to chrisz:

Regarding a)

We could allow setting default_encoding as a mapping from format names to doctype names.

Sounds like a good idea and easy to implement (as you have shown). I'll give it a try.

Regarding b)

1) We could add an optional option keyword parameter with default None to the Buffet render() method

Since the expose decorator already supports it, we could use the mapping parameter for this, or pass its value to the proposed option parameter in view.render.

2) We could somehow pass the docstring along with the format, e.g. separated by a blank or as a tuple. This is similar to how TurboKid passes the Kid-specific output format parameter.

Nah, somehow I don't like this. Don't ask me why...

3) We could pass additional parameters along with the template name in URL query parameter notation, as breve does

I like this. It's a clever, backward-compatible workaround for Buffet's flaws.

Or maybe all of this is not needed, since as a workaround you can pass an instantiated serializer, like this:

from genshi.output import XHTMLSerializer
format = XHTMLSerializer(doctype='xhtml-transitional')

I think this is too complicated for such a mundane task as setting the doctype, but if it works, it's nice to have this option too.

comment:23 Changed 5 years ago by chrisz

Are you working on this? I agree that 2) is not nice, particularly because the second component of the format parameter would then have a different meaning for Kid and Genshi which could be confusing. So we should go with 1) or 3) then, not sure which is better.

comment:24 Changed 5 years ago by Chris Arndt

  • Status changed from new to assigned

Yes, I started an implementation in a new module view/genshisupport.py. I might even try mixing 2) and 3). If you can test setting the format to a serializer class sometime, that would be good.

comment:25 Changed 5 years ago by chrisz

I've now tested passing an initialized serializer object, but unfortunately it doesn't work, because Genshi tries to initialize it again with the default doctype.

So this is another reason for implementing one or two of 1) - 3).

Implementing 1) is most simple, since it is already half-way implemented by the 'mapping' parameter, as you already mentioned.

We would only need to add mapping as an optional parameter to the render method of the Genshi buffet plugin like this:

    def render(self, info, format=None, fragment=False, template=None, mapping=None):
        """Render the template to a string using the provided info."""
        kwargs = self._get_render_options(format=format, fragment=fragment)
        if mapping:
            kwargs.update(mapping)
        return self.transform(info, template).render(**kwargs)

You can then write

@expose(template="...", mapping=dict(doctype='html-transitional'))

We should consider two tweaks here:

  • Rename mapping to options since then it's more clear that it will be used for template options. Renaming will not be a problem because it never worked anyway, since no template engine ever supported a mapping parameter and it was never documented for the Buffet interface.
  • If we want to keep the signature of the render method, we could add another method render_with_options that is called by view.render if there are options. But maybe this only complicates things.
  • Instead of a single parameter mapping receiving a dict, we could use keyword parameters of the form mapping, in the expose decorator and/or in the render method. For the expose method, this would require some more tweaks in turbogears.controllers and view.base, but then you could simply write:
    @expose(template="...", doctype='html-transitional')
    
    Nice, but I'm not sure if this would be too large a change.

comment:26 Changed 5 years ago by chrisz

Hm, just found that we  discussed this already one year ago. Another indicator that this should be finally resolved so we will not discuss it again next year ;-)

comment:27 follow-ups: ↓ 28 ↓ 30 Changed 5 years ago by Chris Arndt

Oh my, what a deja vu!

I think it's clear now, that we should go for choice 1), i.e. passing additional options to TemplateEngine.render() either via an options argument or kwargs.

The remaining decisions are:

  1. Should we change view.render, controllers._process_output and @expose too? (I think yes.)
  2. Should we rename mapping to options in those functions?
  3. If yes, should we keep mappings and issue a deprecation warning if used in TG 1.1?
  4. Or should we use optional kwargs in all those functions? (This might make it difficult to add anything else later on, should the need arise.)

comment:28 in reply to: ↑ 27 Changed 5 years ago by Chris Arndt

Replying to myself:

  1. Or should we use optional kwargs in all those functions?

I just occurs to me, that we can't do this, since option names usually include dots (e.g. "genshi.format"), so they cannot be used as kwargs. We could add the engine name in front of kwarg names, but that's too auto-magical for my taste and would help, if a name contains two dots.

Once decision less ;)

comment:29 Changed 5 years ago by chrisz

This would be a nice argument, but the options for the render method actually do not include dots. At this point in time they are specific to one rendering engine, so they don't contain the engine name, and they are ususally passed as kwargs to the engine's serializer, so they do not contain additional dots. See the example above: You would just say: @expose(template="...", doctype='html-transitional'). The engine is already directly or indirectly specified by the template argument, so not need to repeat it in the options.

Changed 5 years ago by chrisz

Support for options in render method.

comment:30 in reply to: ↑ 27 Changed 5 years ago by chrisz

Replying to Chris Arndt:

  1. If yes, should we keep mappings and issue a deprecation warning if used in TG 1.1?

This parameter was passed directly (not as keyword parameters dict) to the render method. The patch using options does exactly the same, except that it allows arbitrary names, not only "mapping". So if we follow that route, we would be automatically backward compatible concerning "mapping" and don't need to do anything about that.

comment:31 Changed 5 years ago by Chris Arndt

Two things slightly bothering me about this patch:

  1. we're changing the signature of view.render and expose (both public functions) slightly, by shuffling around the position of the arguments. But since mapping and as_format were always documented as keyword arguments, I think that's ok.
  1. Template engines have to handle options passed to their render method differently than those passed to __init__. a) Single arg vs. kwargs (not a problem, I think) a) Option names passed to render will normally not have the plugin-name as a prefix.

comment:32 Changed 5 years ago by chrisz

I had actually posted a comment for motivating this patch, but somehow it got lost. My point was that I had just found that the options approach has actually been used by Pylons 0.9.7 in its  legacy Buffet support already. Buffet support has now been dropped completely, but at least we could consider it as prior art that we would just revive.

Concerning 1., I don't think anybody would use as_format as positional argument since it was at the end of the argument list. Concerning 2., your're right in that options would have a different meaning in init and render. We could rename it to *kwargs to avoid that, but I don't think it's necessary and this can still be done any time without breaking anything.

comment:33 Changed 5 years ago by Chris Arndt

I already had a similar patch in my working copy. I merged the two and committed them in r6759. I also added backwards-compatibility for 'mapping' and a deprecation warning if used. I also added some tests. Please review.

comment:34 Changed 5 years ago by chrisz

Looks good, except for the handling of the mapping parameter (see my comment 30). If you look at how "mapping" has been used in the view.render() method before, you will see that it has been passed directly to the buffet render() method as a single parameter, not resolved as a dict of keyword parameters. That's why I wrote we can simply ignore it because it's already covered by the **options approach. So we don't even need any deprecation warnings (and I'm sure nobody used it anyway because it never had been useful that way).

comment:35 Changed 5 years ago by Chris Arndt

Unfortunately, I had to revert r6759. test_identity:test_gettingjson was failing. Somehow _execute_func received options=False instead of an empty dict. May have something to do with dispatch rules. Will investigate more tomorrow.

comment:36 follow-up: ↓ 37 Changed 5 years ago by chrisz

The only problem in r6759 was that _execute_func is called two times in turbogears.controllers, and we forgot to adapt the first of these calls (in the _build_rules function). There, we need to change

                _execute_func(_func, "json", "json", "application/json",
                    None, False, args, kw))

to

                _execute_func(_func, "json", "json", "application/json",
                    False, {}, args, kw))

If you like, I can check the patch in again with this fix. As explained above, I would also remove the special casing for the "mapping" parameter.

comment:37 in reply to: ↑ 36 Changed 5 years ago by Chris Arndt

Replying to chrisz:

If you like, I can check the patch in again with this fix.

Yes, please.

As explained above, I would also remove the special casing for the "mapping" parameter.

Yes, you had me convinced this was unnecessary and I was about to commit his change when I discovered the failing test. Just remove the try/except clauses and the last part of the test in test_expose.py.

comment:38 Changed 5 years ago by chrisz

Ok, re-applied the patch in r6783.

Now we still have 2 open issues:

  • Monkey-patching or replacing the Genshi Buffet interface so that our render **options are used.
  • Automatically using appropriate doctypes for different formats by default. The idea was to allow setting default_encoding as a mapping for that purpose.

comment:39 Changed 5 years ago by Chris Arndt

I'm on it, will commit sth. tomorrow.

comment:40 Changed 5 years ago by Chris Arndt

  • Keywords documentation added; tests removed
  • Status changed from assigned to closed
  • Resolution set to fixed
  • Milestone changed from 1.1 to 1.0.x bugfix

Phew, I think I finally fixed this. See r6787. Tests are in r6788.

This opened up another can of worms, though (see commit message), so I had to remove the genshi.encoding option, which didn't work anyway. You now have to use genshi.default_encoding and that's what the Genshi documentation tells you anyway. If you have used this option before, i.e set it somthing other than "utf-8", you need to change the setting name.

This latter change should be backported to TG 1.1 too.

comment:41 Changed 5 years ago by Chris Arndt

  • Milestone changed from 1.0.x bugfix to 1.1

comment:42 Changed 5 years ago by chrisz

Backported the correction of encoding to default_encoding to TG 1.0 in r6805. The other improvements for Genshi users will not be backported as these are new features; if you want to use Genshi, you should upgrade to TG 1.1 anyway.

Note: See TracTickets for help on using tickets.