Skip to content

Commit

Permalink
Drop 'ZopeFieldStorage' in favor of 'cgi.FieldStorage'.
Browse files Browse the repository at this point in the history
The stdlib bug which caused us to fork it is long gone, and we have
our own Py3k bug now.

Leave behind a BBB alias, in case anybody is expecting to import it.
  • Loading branch information
tseaver committed Dec 5, 2017
1 parent 4669b8e commit e2f83e6
Showing 1 changed file with 3 additions and 36 deletions.
39 changes: 3 additions & 36 deletions src/ZPublisher/HTTPRequest.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,12 @@
""" HTTP request management.
"""

from cgi import FieldStorage
import cgi
import codecs
import collections
from copy import deepcopy
import os
from os import unlink
from os.path import isfile
import random
import re
from tempfile import (
mkstemp,
_TemporaryFileWrapper,
)
import time

from AccessControl.tainted import TaintedString
Expand Down Expand Up @@ -511,7 +504,7 @@ def processInputs(
environ['QUERY_STRING'] = ''

meth = None
fs = ZopeFieldStorage(fp=fp, environ=environ, keep_blank_values=1)
fs = cgi.FieldStorage(fp=fp, environ=environ, keep_blank_values=1)

# Keep a reference to the FieldStorage. Otherwise it's
# __del__ method is called too early and closing FieldStorage.file.
Expand Down Expand Up @@ -1641,34 +1634,8 @@ def sane_environment(env):
return dict


class TemporaryFileWrapper(_TemporaryFileWrapper):
"""
Variant of tempfile._TemporaryFileWrapper that doesn't rely on the
automatic Windows behavior of deleting closed files, which even
happens, when the file has been moved -- e.g. to the blob storage,
and doesn't complain about such a move either.
"""

if PY2:
unlink = staticmethod(unlink)
isfile = staticmethod(isfile)

def close(self):
if not self.close_called:
self.close_called = True
self.file.close()

def __del__(self):
self.close()
if self.isfile(self.name):
self.unlink(self.name)


class ZopeFieldStorage(FieldStorage):
ZopeFieldStorage = cgi.FieldStorage # BBB

def make_file(self, binary=None):
handle, name = mkstemp()
return TemporaryFileWrapper(os.fdopen(handle, 'w+b'), name)

# Original version: zope.publisher.browser.FileUpload
class FileUpload(object):
Expand Down

4 comments on commit e2f83e6

@pbauer
Copy link
Member

@pbauer pbauer commented on e2f83e6 Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit breaks a couple of Archetypes-Tests in Plone 5.2. See http://jenkins.plone.org/job/plone-5.2-python-2.7-at/143/

E.g. after processing a fileupload in plone.app.blob request.form['file'].name is the string <fdopen> instead of a filepath (like /var/folders/n3/....). This example is from http://jenkins.plone.org/job/plone-5.2-python-2.7-at/143/testReport/junit/plone.app.blob.tests.test_integration/IntegrationTests/testSize/

@hannosch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the monkey patch in https://github.com/plone/plone.app.blob/blob/master/src/plone/app/blob/monkey.py#L56 doesn’t trigger anymore. The code in Zope is now instantiating the field storage as cgi.FieldStorage. The monkey patch only overwrites FieldStorage and ZopeFieldStorage.

I think we could change the import back in Zope, so it directly does from cgi import FieldStorage. In that case the monkey patch should still work.

@pbauer
Copy link
Member

@pbauer pbauer commented on e2f83e6 Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hannosch Thanks. I did what you suggested in #235.
Also: In plone/Products.CMFPlone#2193 (comment) @mauritsvanrees suggested that the patch could go away altogether.

@mauritsvanrees
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have merged the PR.

Please sign in to comment.