search

zopefoundation / zope.publisher

Map requests from HTTP/WebDAV clients, web browsers, XML-RPC and FTP clients onto Python objects
Other
3 stars 12 forks source link

Port form data parsing to multipart #55

Closed cjwatson closed 3 years ago

cjwatson commented 4 years ago

This currently adds ResourceWarnings on Python 3; that will be fixed when https://github.com/defnull/multipart/pull/21 lands.

Fixes #39. Closes #54.

See also my alternative solution in #54. (I prefer this one.)

cjwatson commented 4 years ago

I'm going to leave this a week or so to see if anyone has other thoughts, and then land this in the absence of objections.

cjwatson commented 4 years ago

I ran this though Launchpad's test suite, which caught a couple of mistakes. I've pushed some fixes for those and would appreciate re-review.

It may be necessary to wait for defnull/multipart#25 and a Python 2 equivalent before landing this. Without this, Launchpad's test suite regresses in a way that would be hard to work around in zope.publisher. I don't know how widespread the effect of this would be on other users of zope.publisher; it affects us due to what's arguably a bug in wadllib, but that bug was inherited from using the standard library's email.mime to construct multipart/form-data request bodies, so it's possible others have made a similar mistake.

icemac commented 3 years ago

Everybody voted for this PR over #54. @mgedmin Could you please re-review the last changes so it can eventually be merged?

icemac commented 3 years ago

defnull/multipart#21 has landed in the release 0.2.2 of multipart.

cjwatson commented 3 years ago

I agree this is preferable to #54, but just note that we still need defnull/multipart#25 to land (and a Python 2 equivalent) first. The multipart maintainer offered me co-maintenance which I'm happy to do, but I'm waiting for the actual corresponding permission bits before I can move forward with that.

jugmac00 commented 3 years ago

I agree this is preferable to #54, but just note that we still need defnull/multipart#25 to land (and a Python 2 equivalent) first. The multipart maintainer offered me co-maintenance which I'm happy to do, but I'm waiting for the actual corresponding permission bits before I can move forward with that.

That is great news! I was a bit worried to rely on an external package, which only one person has access to, and is not seemingly not very active. But with you being a co-maintainer, this sounds excellent! Thanks for all your work you already put into this issue!

icemac commented 3 years ago

https://github.com/defnull/multipart/pull/25 Seems to be merged. What are the next steps?

cjwatson commented 3 years ago

I've released multipart 0.1.1 and 0.2.3 now, so I'm going to go ahead and land this. Thanks all.