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 #2194 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

Turbogears overrides controller return values for templates

Reported by: kikidonk Owned by: faide
Priority: highest Milestone: 2.0b6
Component: TurboGears Version: trunk
Severity: normal Keywords:
Cc:

Description

Because of http://trac.turbogears.org/browser/trunk/tg/render.py#L125 If a controller returns any value which name collides with one of the builting template variable defined in _get_tg_vars, that value will be overwritten and confusion will ensue.

For example if a controller returns

@expose('foobar.html')
def foo(self):
    return {'url': 'http://www.turbogears.org}

An attempt to use ${url} in the foobar template will result in tg.controllers.url being returned.

I guess the fix would be to prefer user-returned values over default tg_vars, instead of template_vars.update(_get_tg_vars()) maybe use something like final_vars = _get_tg_vars(); final_vars.update(template_vars)

Change History

comment:1 Changed 10 years ago by mramm

  • Priority changed from normal to highest

Agreed, this is a simple enough change. But it is an (abet undocumented) API change, so it will need to happen this week.

comment:2 Changed 10 years ago by jorge.vargas

I don't like this. what if you overwrite something important? say session or identity or the request?

comment:3 Changed 10 years ago by faide

we should provide the url function (and other variables) inside the tg namespace.

And then warn the user to not use the tg variable in the returned dict

this would avoid this situation.

comment:4 follow-up: ↓ 9 Changed 10 years ago by chrisz

url is already inside tg, but it is outside as well; the same is true for request; response is only outside tg, config only inside. I have the feeling that this is all pretty arbitrary and really needs some clean up.

At the top level I think we should only have tg and the one-letter-variables from Pylons.

comment:5 Changed 10 years ago by faide

that's my point...

comment:6 Changed 10 years ago by kikidonk

Note that there's also the bug #2193 where the 'h' variable conflicts with mako's 'h' filter, so if you let one-letter-variables from pylons at the root level it will still cause conflicts.

That said, I'm all for namespacing all the stuff that's now top-level for consistency's sake.

Also from my point of view, user-returned variables from a controller should always overwrite tg-added variables because that's the least surprising thing to do. The current situation is that i return {'url': 'foo'} and the template fails to render because 'URLGenerator has no member strip' and that's really surprising...

comment:7 Changed 10 years ago by faide

We should get to a decision quickly here. Kiki is right, in the fact that if a user returns a variable from a controller he expects to find it on the other side (template).

I think we should namespace all our provided variables and functions in tg.* to make sure the user cannot overwrite accidentally something. And we should protect the tg.* namespace by inspecting the returned dict from the user and raising an execption if the user returned a varible named 'tg'.

I prefer to break API quickly before b6 on this important issue if we need to.

comment:8 Changed 10 years ago by jorge.vargas

I think we will need the namespacing, It's the most obvious way to get around this. Now keep in mind both pylons and TG are doing this. I found a related issue working with another ticket, as you can tell my quick-dirty patch is dirty mainly because of this namespacing issue http://trac.turbogears.org/ticket/2212

Which is why I'm not totally sold on the namespace because we'll have to store, all of the following variables in there. ['tg','h','helpers','c','url','request','response','tmpl_context']

I think we should consider other options

1- reserved keywords, we'll prevent users from using vars TG itself is using (generating an error) 2- overwrite TG defaults by user vars (warning if a user sends a response variable to the template things will get screwed, or h, that will break even in more colourful wants than url.

comment:9 in reply to: ↑ 4 Changed 10 years ago by mramm

Replying to chrisz:

url is already inside tg, but it is outside as well; the same is true for request; response is only outside tg, config only inside. I have the feeling that this is all pretty arbitrary and really needs some clean up.

At the top level I think we should only have tg and the one-letter-variables from Pylons.

The reason for this is that pylons places the url, request, and response outside, so all "root" level variables in the template are there to match what Pylons does by default. And all the ones inside are copies of what was in tg1.

I am more than willing to put everything inside of tg, and document it as different from both tg1 and tg2, but there is reasoning behind why it is the way it is now.

comment:10 Changed 10 years ago by mramm

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

Ok, so now user vars from the return dictionary override the ones provided by turbogears automatically.

Note: See TracTickets for help on using tickets.