Delete page exhibits same behaviour as delete version

[trac-form@…]

When I "delete page", it displays the delete page confirmation page, but only deletes the current version.

Delete version URL:

http://trac-hacks.swapoff.org/wiki/FooBar?action=delete&version=2&delete_version=Delete+this+version

Delete page URL:

http://trac-hacks.swapoff.org/wiki/FooBar?action=delete&version=2

If I manually remove the &version=2 from the URL, it deletes as expected.

[trac-form@…]

I modified trac/wiki/web_ui.py to raise a TracError if version is defined, and it did indeed raise. Matt suggested that perhaps I have an older version, but I just did a fresh checkout of trunk and reinstalled after rm -rf'ing /usr/lib/python-2.4/site-packages/trac. I also suspected it might be the Tags wiki.cs override, but I tried it with stock wiki.cs to no avail.

    def _do_delete(self, req, db, page):
        if page.readonly:
            req.perm.assert_permission('WIKI_ADMIN')
        else:
            req.perm.assert_permission('WIKI_DELETE')

        if req.args.has_key('cancel'):
            req.redirect(self.env.href.wiki(page.name))

        version = None
        if req.args.has_key('version'):
            version = int(req.args.get('version', 0))
            raise TracError("%i" % version)

[trac-form@…]

Presumably this is occurring because the confirmation form has action="":

  <h1>Delete <a href="/wiki/FooBar">FooBar</a></h1>

  <form action="" method="post">
   <input type="hidden" name="action" value="delete" />
   <p><strong>Are you sure you want to completely delete this page?</strong><br />
   This is an irreversible operation.</p>
   <div class="buttons">
    <input type="submit" name="cancel" value="Cancel" />
    <input type="submit" value="Delete page" />
   </div>

  </form>

The URL has "version=2" in it, which then gets picked up by if req.args.has_key('version') as a sign to delete a specific version. The following code change fixes the issue:

trac/wiki/web_ui.py

@@ -157 +157 @@
             req.redirect(self.env.href.wiki(page.name))
 
         version = None
-        if req.args.has_key('version'):
+        if req.args.has_key('delete_version') and req.args.has_key('version'):
             version = int(req.args.get('version', 0))
 
         page.delete(version, db)

[mgood]

Well, apparently with the CGI and FastCGI front-ends req.args contains only POST parameters when the request method is POST, but the mod_python front end is wrapping the request in a way that preserves the GET arguments as well. It seems like the cleanest thing would be to make mod_python conform to the CGI behavior. This means that POSTs wouldn't be able to access any GET parameters on the URL, though mixing the two should probably be avoided anyways and CGI and FastCGI seem to be working fine as they are now.

Unless there are any objections this patch should fix the mod_python frontend:

trac/web/modpython_frontend.py

@@ -138 +138 @@
         a POST request. We work around this to provide the behaviour of cgi.py
         here.
         """
-        class RequestWrapper(object):
-            def __init__(self, req):
-                self.req = req
-                self.args = ''
-            def __getattr__(self, name):
-                return getattr(self.req, name)
-        util.FieldStorage.__init__(self, RequestWrapper(req), keep_blank_values=1)
+        if req.method == 'POST':
+            class RequestWrapper(object):
+                def __init__(self, req):
+                    self.req = req
+                    self.args = ''
+                def __getattr__(self, name):
+                    return getattr(self.req, name)
+            req = RequestWrapper(req)
+        util.FieldStorage.__init__(self, req, keep_blank_values=1)
 
-        # Populate FieldStorage with the original query string parameters, if
-        # they aren't already defined through the request body
-        if req.args:
-            qsargs = []
-            for pair in util.parse_qsl(req.args, 1):
-                if self.has_key(pair[0]):
-                    continue
-                qsargs.append(util.Field(pair[0], StringIO(pair[1]),
-                                         "text/plain", {}, None, {}))
-            self.list += qsargs
-
     def get(self, key, default=None):
         # Work around a quirk with the ModPython FieldStorage class.
         # Instances of a string subclass are returned instead of real

[cmlenz]

That behavior of the mod_python frontend was introduced in [1467] to fix #1370:

The problem here is that the FieldStorage? implementation of mod_python behaves differently from the implementation in cgi.py. While the latter adds query-string parameters after POST body parameters, and does so only if the the parameter isn't already in the body, the mod_python implementation adds query string parameters first and unconditionally.

Looking at the Python cgi module (in 2.3), I think this analysis is correct, at least for forms that aren't multipart encoded:

    if environ['REQUEST_METHOD'] == 'POST':
        ctype, pdict = parse_header(environ['CONTENT_TYPE'])
        if ctype == 'multipart/form-data':
            return parse_multipart(fp, pdict)
        elif ctype == 'application/x-www-form-urlencoded':
            clength = int(environ['CONTENT_LENGTH'])
            if maxlen and clength > maxlen:
                raise ValueError, 'Maximum content length exceeded'
            qs = fp.read(clength)
        else:
            qs = ''                     # Unknown content-type
        if 'QUERY_STRING' in environ:
            if qs: qs = qs + '&'
            qs = qs + environ['QUERY_STRING']
        elif sys.argv[1:]:
            if qs: qs = qs + '&'
            qs = qs + sys.argv[1]
        environ['QUERY_STRING'] = qs    # XXX Shouldn't, really
    elif 'QUERY_STRING' in environ:
        qs = environ['QUERY_STRING']

So, for a form with the standard application/x-www-form-urlencoded encoding, the parameters in the request body are simply appended to the query string, which is then later parsed to populate the FieldStorage fields. Maybe this has been changed in Python 2.4?

[cmlenz]

I'm going to fix this for now by simply correcting the action URL of the form.

[cmlenz]

Fixed in [2578] and [2579].