Pages of quickstarted TG 1.1 app do not validate

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.

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.

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

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.

2. 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.

3. 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.

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.

Implemented preliminary solution 3) in r5339.

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.

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.

Since r5339 'html' is the default.

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".

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.

closing as wontfix...

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.

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.

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.

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?

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.

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?

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.

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.

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.

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.

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.

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

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

Replying to myself:

4. 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 ;)

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.

Support for **options in render method.

Replying to Chris Arndt:

3. 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.

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.

2. 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.

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.

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.

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

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.

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.

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.

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.

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

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.

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.