Doesn't

+    else {
+        /* default - only act if starts-with "text/" or contains "xml" */
+        wanted = !strncmp(ctype, "text/", 5) || strstr(ctype, "xml");
+    }

suffer from the same problem as the original code ? So if the user did not
give any "xml2Types" the default behaviour will hit the same problem as
with http://www.mail-archive.com/dev@httpd.apache.org/msg57029.html (the
mentioned earlier discussion regarding ctypes). IMO, this is especially
important seeing how using the new code requires configuration entries to
be added which a lot of admins will either not be aware of or simply not be
doing. In regards to the cited patch section above, I suggest something
along the lines of my patch suggestion in the first message of the
mentioned thread, e.g. include
  !strncmp(ctype, "application", 11)

Otherweise it's fine I think. Will test it sometime this week.



On Thu, Feb 6, 2014 at 10:40 AM, Ewald Dieterich <
ewald_dieter...@t-online.de> wrote:

> Thanks for the patch!
>
>
> On 02/05/2014 02:57 PM, Nick Kew wrote:
>
>>
>> The hesitation is because I've been wanting to review the
>> patch before committing, and round tuits are in woefully
>> short supply.  So I'm attaching it here.  I'll take any feedback
>> from you or other users as a substitute for my own review,
>> and commit if it works for you without glitches.
>>
>
> Minor glitch: the patch doesn't compile because it uses the unknown
> variable cfg in xml2enc_ffunc(). Otherwise it works as advertised.
>
> My wishlist:
>
> * Make the configuration option as powerful as the compiled in fallback so
> that you can configure eg. "contains xml". But how would you do that?
> Support regular expressions?
>
> * Provide a configuration option to blacklist content types so that you
> can use the defaults that are compiled in but exclude specific types from
> processing (this is how I work around the Sharepoint problem, I simply
> exclude content type "multipart/related").
>

Reply via email to