post_process_request (again) following r6691

[osimons]

[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

@@ -287 +287 @@
         return chosen_handler
 
     def _post_process_request(self, req, *args):
+        nbargs = len(args)
         resp = args
         for f in reversed(self.filters):
-            # as the arity of `post_process_request` has changed since 
-            # Trac 0.10, we only call filters which have the same arity
-            if arity(f.post_process_request) - 2 == len(args):
+            # As the arity of `post_process_request` has changed since 
+            # Trac 0.10, only filters with same arity gets passed real values.
+            # Other calls are made with None arguments (including error
+            # situations that call without arguments), and results not saved.
+            extra_arg_count = arity(f.post_process_request) - 2
+            if extra_arg_count == nbargs:
                 resp = f.post_process_request(req, *resp)
+            else:
+                f.post_process_request(req, *(None,)*extra_arg_count)
         return resp
 
 

OK to commit?

[Christian Boos]

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.

[osimons]

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… :-)

[Christian Boos]

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

[Noah Kantrowitz]

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.

[John Hampton]

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

[osimons]

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)

[Noah Kantrowitz]

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)

[osimons]

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

I'll get trunk fixed up soon-ish.

[Christian Boos]

svnmerge'd in [7037].