[patch] New ticket details are not passed from Trac error pages

[pkou@…]

This is a follow-up to #2945, r4347, r4387

When new ticket is created, it can populate fields from the request. However, formatting rules are changed: instead of name=value it is necessary to use field_name=value.

This change has not been applied to all places.

Fix:

trac/templates/error.html

@@ -33 +33 @@
 
   <py:def function="create_ticket(teo=False)">
     <input type="hidden" name="preview" value="1" />
-    <input type="hidden" name="reporter" value="${get_reporter_id(req)}" />
-    <input py:if="teo" type="hidden" name="version"
+    <input type="hidden" name="field_reporter" value="${get_reporter_id(req)}" />
+    <input py:if="teo" type="hidden" name="field_version"
            value="${'dev' in trac.version and 'devel' or trac.version}" />
-    <input type="hidden" name="summary" value="$message" />
-    <textarea name="description">
+    <input type="hidden" name="field_summary" value="$message" />
+    <textarea name="field_description">
 ==== How to Reproduce ====
 
 While doing a $req.method operation on `$req.path_info`, Trac issued an internal error.

trac/wiki/tests/wikisyntax.py

@@ -277 +277 @@
 </p>
 ------------------------------
 ============================== Relative to the base url
-[/newticket?priority=high bug]
+[/newticket?field_priority=high bug]
 [/ Project]
 ------------------------------
 <p>
-<a href="/trac/newticket?priority=high">bug</a>
+<a href="/trac/newticket?field_priority=high">bug</a>
 <a href="/trac">Project</a>
 </p>
 ------------------------------

Probably, TracTickets#PresetValuesforNewTickets requires the update, also.

[Christian Boos]

I think we can apply the patch for consistency, even it's not completely settled at this point if we should keep that naming (see #4947).

[pkou at ua.fm]

It seems that the proposed naming in #4947 (rename version to revision) is better than having field_xxx for ticket fields.

[Christian Boos]

Thinking a bit more about this, I believe the /newticket and /ticket requests don't necessarily have to have the same conventions.

Spelling out /newticket URLs is quite common usage, and being able to use directly the ticket fields without a prefix is convenient. Also, there's no need for a version parameter here, or a format one or any other kind of special parameter.

OTOH, when passing parameters to ticket URLs or links, what we expect to use are the special parameters like version or format. Being able to specify a ticket field has little if any use there, so it's OK if a field_ prefix is expected for field parameters in this situation.

[Christian Boos]

t.e.o should not be upgrade to 0.11 until the /newticket processing gets fixed like exlained above, otherwise creating semi-automated bug reports won't work.

[Christian Boos]

(also don't forget to adapt the ticket clone code in the template to the 0.10 convention)

[ThurnerRupert]

is there a special reason why this does not get applied?

[anonymous]

Any status to report? This appears to be the critical path item.

[anonymous]

Agree that more information would be helpful to understand. Can all / some of this be deferred in the interested of releasing 0.11?

[Christian Boos]

This ticket needs to be resolved before we upgrade t.e.o to 0.11dev, as otherwise the semi-automatic bug reporting mechanism will not work. The way to fix it is described above in comment:4, so fixing the ticket is only a matter of time.

[Christian Boos]

Fixed in r6120.

[Noah Kantrowitz]

This is not an acceptable fix. All thing related to the old field_ names should be removed rather than just changing the request magically.

[Christian Boos]

What are you suggesting exactly? Removing the field_ prefix in the templates?

Also, can you elaborate why the fix is not "acceptable" according to you? The way you put it doesn't look very constructive. I'd suggest you first try to find a better approach, come with a patch, then we can discuss it.

[Noah Kantrowitz]

This is not acceptable because there are other plugins that don't appreciate having their request args changed behind the scenes. Specifically this makes the template debugger unusable. This is very much a blocker issue, it is an incredibly obtrusive mechanism. If we are not using the field_ prefix, it needs to go away entirely not just hidden.

[Christian Boos]

Replying to nkantrowitz:

… If we are not using the field_ prefix, it needs to go away entirely not just hidden.

Well, not at all, the field_ prefix is used, of course. I think you got it backwards, we're not hidding the field_ prefix, we're adding it when it's not there in very specific circumstances.

The only situation where a parameter mapping will take place is for GET requests to /newticket, as the only thing that will trigger such requests are hand-written URLs or TracLinks (like [/newticket?component=thing file a bug for the thing]). See r6120 / r6122.

So for newticket POSTs and ticket POSTs that happen during normal use, there's no parameter change under the hood.

There are two alternatives for not having this special case:

• not having the field_ prefix at all - this is the 0.10 situation, and the problem with that approach, besides the conflict of meaning between version (field or ticket version?) is that it's error prone as custom fields may clash with any "reserved" parameter
• having the field_ prefix everywhere, including for /newticket URLs. This has the disadvantage of invalidating all pre-existing hand-written URL, and is also a bit cumbersome to write

Both have disadvantages, so that's why I came up with the middle ground approach (comment:4) which at that time sounded OK (IIRC cmlenz reviewed the patch). And that's why I asked (genuinely) if you had a better idea.

[Christian Boos]

Taking into account the constraints exposed above, I see one solution which could work: in the /newticket match_request, only rename the parameters matching actual field names, leaving the others alone.

Would that be enough for the plugins?

[Noah Kantrowitz]

The easiest way to handle this will probably be to handle the translation inside process_request, and also to make a new dict with the modified arguments instead of destructively changing req.args. This should at least prevent the current insanity from leaking out to the world.

[Christian Boos]

Don't modify req.args in match_request

[Christian Boos]

Please have a look at the patch. I didn't follow your suggestion to make a new dict, as req.args was already used throughout the whole Ticket module and passing that new dict around would have required too many changes.

[Noah Kantrowitz]

This still changes req.args, so I don't know why you think this is any better. Pre-request filters are okay now, but post-request and stream filters are both in the same situation. Just do a s/req.args/args/, on that file or something, or find a real way to fix this rather than just moving the problem around.

[ebray <hyugaricdeau@…>]

Right, this would still allow allow custom fields to clobber reserved names. I preferred the other way better, though nkantrowitz has valid concerns.

What about just leaving everything alone for the common case (build-in fields), so don't mess with req.args at all. And for custom fields (less common) require something annoying, like that they be prefixed with an underscore (for example).

I have some users who would be confused by this, but maybe a reasonable middle ground would be something like that.

[Christian Boos]

Replying to nkantrowitz:

This still changes req.args, so I don't know why you think this is any better.

Because only req.args entries corresponding to field names are normalized, any others are left unchanged.

Pre-request filters are okay now, but post-request and stream filters are both in the same situation.

Well, I'm not sure why you think it's better for post-request and stream filters to see the original parameters instead of the normalized ones. Let's say there is a reason, and we make it as a rule that process_request should not alter the req.args - but please note that was not the case so far (e.g. the del req.args['field_owner'] in _process_newticket_request). Also, simply adding to the req.args is even common practice in most of the match_request.

Just do a s/req.args/args/, on that file or something, or find a real way to fix this rather than just moving the problem around.

Ok, so we would definitely need to do that if we want to preserve req.args, not only for the problem discussed here, but also for the #3295 case (where the del req.args['owner'] change was originally introduced).

[Christian Boos]

Try another approach, without touching to the req.args

[Christian Boos]

Above patch committed in r6834. There's also r6835, which re-introduces a check for /newticket?id=xyz (#5022), which is again needed.