Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

#7185 closed defect (fixed)

post_process_request (again) following r6691

Reported by: osimons Owned by: osimons
Priority: high Milestone: 0.11
Component: general Version: 0.11rc1
Severity: normal Keywords: post_process_request
Cc: pacopablo@…, Noah Kantrowitz Branch:
Release Notes:
API Changes:

Description

[6691] made some changes to trac.web.main.RequestDispatcher._post_process_request() that has an unfortunate side-effect of not calling filters when errors occur.

If errors are detected, filters are just called as self._post_process_request(req) which means that the argument count will never match, and hence no calls will actually be made.

As all filters for a long time has been able to handle calls with None argument replacements, I suggest we revert this back partially so that whenever there is not a direct match on arguments ('normal' processing for same version filters), we call and pass None arguments and discard the result.

The diff looks like this:

  • trac/web/main.py

     
    287287        return chosen_handler
    288288
    289289    def _post_process_request(self, req, *args):
     290        nbargs = len(args)
    290291        resp = args
    291292        for f in reversed(self.filters):
    292             # as the arity of `post_process_request` has changed since
    293             # Trac 0.10, we only call filters which have the same arity
    294             if arity(f.post_process_request) - 2 == len(args):
     293            # As the arity of `post_process_request` has changed since
     294            # Trac 0.10, only filters with same arity gets passed real values.
     295            # Other calls are made with None arguments (including error
     296            # situations that call without arguments), and results not saved.
     297            extra_arg_count = arity(f.post_process_request) - 2
     298            if extra_arg_count == nbargs:
    295299                resp = f.post_process_request(req, *resp)
     300            else:
     301                f.post_process_request(req, *(None,)*extra_arg_count)
    296302        return resp
    297303
    298304

OK to commit?

Attachments (0)

Change History (10)

in reply to:  description ; comment:1 by Christian Boos, 11 years ago

Keywords: post_process_request added
Priority: normalhigh

Replying to osimons:

If errors are detected, filters are just called as self._post_process_request(req) which means that the argument count will never match, and hence no calls will actually be made.

Wouldn't it be better to only call the non-matching filters in that case only? i.e. elif nbargs == 0: instead of else:. That would call both the Genshi and ClearSilver filters in the case of an error, but not a Genshi filter when processing a ClearSilver template and vice versa.

in reply to:  1 ; comment:2 by osimons, 11 years ago

Replying to cboos:

Wouldn't it be better to only call the non-matching filters in that case only? i.e. elif nbargs == 0: instead of else:. That would call both the Genshi and ClearSilver filters in the case of an error, but not a Genshi filter when processing a ClearSilver template and vice versa.

We could. But that breaks a symmetry in that a ClearSilver plugin would never get called except in error situations - at least in regular 0.11+ request handling. This way all plugins always gets called, but unless the plugin is of the right version it does not get meaningful input except req, and output is discarded.

However, I'll be fine with either way as I suspect there won't be many pure 0.10 plugins left working correctly on 0.11 anyway. At least I have none. And, skipping them would be a good incentive to get them updated, although it would no longer be possible to make a plugin that supports both 0.10 and 0.11. You decide and I'll commit?

PS! Yes, I do remember you showed me the patch before committing. Sorry for being short-sighted and failing to spot the consequences for error handling and so on back then… :-)

in reply to:  2 comment:3 by Christian Boos, 11 years ago

Replying to osimons:

And, skipping them would be a good incentive to get them updated, although it would no longer be possible to make a plugin that supports both 0.10 and 0.11.

How could a given Component support both, as the "choice" is based on the arity of the method?

You decide and I'll commit?

Well, I'd prefer the way I suggested, so that the plugin can actually tell that when its arguments are None, it's because it is called when post-processing an error page and not eventually because it's a normal processing but for the "other" template engine.

This also means that the API doc should be updated to mention this situation, e.g.

Note that `data` and `template` will both be `None` in case:
 - an exception was raised and caught, and the post-process filter is executed before rendering the error page"
 - the default request handler did not return any result

comment:4 by Noah Kantrowitz, 11 years ago

Given how things have worked out, the 0.10-style request filters are now more likely to cause bugs than be helpful. I don't know of anywhere they are actually being used usefully, so I am in favor of dropping the legacy support before releasing 0.11.

comment:5 by John Hampton, 11 years ago

Cc: pacopablo@… added

I'm for elif nbargs == 0:

I doubt that there is anyone with a 0.10 plugin that runs on error and a 0.11 plugin that runs on error and needs both of them to be called. If there happens to be that person, then they can port the 0.10 plugin.

Of course, I also think this change needs to go into 0.11-stable

comment:6 by Noah Kantrowitz, 11 years ago

Cc: Noah Kantrowitz added

comment:7 by osimons, 11 years ago

Right, how about this for a proposal then:

  • Use elif nbargs == 0 for 0.11-stable
  • Remove the old API in trunk (where really all leftovers from ClearSilver can soon be removed)

comment:8 by Noah Kantrowitz, 11 years ago

Okay, tested the amended patch and it is working. +1 on commit. (I would commit it myself, but the access rights for 0.11-stable are still b0rked)

comment:9 by osimons, 11 years ago

Committed in [7003] for 0.11-stable (with nbargs == 0 + updated docs).

I'll get trunk fixed up soon-ish.

comment:10 by Christian Boos, 11 years ago

Resolution: fixed
Status: newclosed

svnmerge'd in [7037].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from osimons to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.