Opened 11 years ago

Closed 5 years ago

#516 defect closed fixed (fixed)

deprecate PHP3Script and PHPScript in twisted.web.twcgi, replace them with documentation for FilteredScript

Reported by: Jerub Owned by:
Priority: high Milestone:
Component: web Keywords:
Cc: radix, moshez, Jerub, exarkun, thijs Branch: branches/phpscript-deprecation-516
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description


Attachments (1)

patch-twcgi (823 bytes) - added by Jerub 11 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 11 years ago by Jerub

/usr/bin/php4 in debian unstable is the 'cli' version, a misguided product of a
push to see php in areas such as the command line.

the location of the cgi binary is in /usr/lib/cgi-bin/php4 - and in order for
the CGI version to function correctly, the env variable 'REDIRECT_STATUS' must
be '200'.

This patch probably isn't *optimal*, and will likely break other deployments
where /usr/bin/php4 is the cgi version and the other path doesn't actually
exist, but it demonstrates exactly what is required at a code level for this to
function on debian unstable.

Changed 11 years ago by Jerub

comment:2 Changed 11 years ago by itamarst

I suggest this patch get applied to debian packaged versions of Twisted when
they get generated.

comment:3 Changed 8 years ago by exarkun

  • Priority changed from high to low

I suppose it would be good to have support for PHP4 CGIs on Debian. Did anyone contact Debian to discuss this issue? Also, couldn't we check for the existence of various executables instead of just assuming /usr/bin/php4 exists and is usable?

In any case, this isn't high priority, and the change needs test coverage.

comment:4 Changed 6 years ago by exarkun

  • Cc exarkun added
  • Priority changed from low to normal
  • Summary changed from [PATCH] twcgi is non-functional with debian unstable's php4-cgi to deprecate PHP3Script and PHPScript in twisted.web.twcgi, replace them with documentation for FilteredScript

PHP3Script and PHPScript are nothing except platform-specific configuration. If we can't get this right, we shouldn't have it. We can have documentation which explains how to trivially create one of these which is correct for a particular location of the php cgi interpreter.

comment:5 Changed 5 years ago by exarkun

  • Owner changed from moshez to jesstess
  • Priority changed from normal to high

This fruit hangs low.

comment:6 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/phpscript-deprecation-516

(In [28136]) Branching to 'phpscript-deprecation-516'

comment:7 Changed 5 years ago by jesstess

(In [28146]) Deprecate PHPScript and PHP3Script and add unit tests for the deprecation.

refs #516

comment:8 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

I also added rather mundane docstrings for FilteredScript.runProcess and CGIScript.runProcess; more interesting descriptions are welcome.

comment:9 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

Thanks! The docstring additions look good. A couple other things:

  • There's one pyflakes warning for the module, an unused local in CGIScript.render. Can you fix that?
  • I think a brief section in the prose documentation for CGIScript and FilteredScript would be a good addition as well. I want to suggest adding it to using-twistedweb.xhtml, but it also occurs to me that it could be a section in the new sixty seconds documentation except that that branch hasn't been merged yet. Blocking this ticket on the sixty seconds ticket sounds lame. I do want to push us towards writing more prose documentation, though. Maybe I should do a sixty seconds post on twcgi and we can add it to the sixty seconds branch (or to another ticket after the sixty seconds branch is merged). (Sorry for the rambling non-committal review point.)

comment:10 Changed 5 years ago by jesstess

(In [28172]) Remove unused local variable and clean up surrounding doc strings.

refs #516

comment:11 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Ticket #4273 was opened to address exarkun's second review comment.

comment:12 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

Oops, I missed some test failures before. Looks like the new __init__ methods aren't compatible with the inherited __init__:

===============================================================================
[ERROR]: twisted.web.test.test_tap.ServiceTests.test_php3Processor

Traceback (most recent call last):
  File "/srv/bb-slave/Run/slave/reactors/Twisted/twisted/web/test/test_tap.py", line 80, in test_php3Processor
    self.assertIsInstance(root.getChild("foo.php3", None), PHP3Script)
  File "/srv/bb-slave/Run/slave/reactors/Twisted/twisted/web/static.py", line 311, in getChild
    return resource.IResource(processor(fpath.path, self.registry))
exceptions.TypeError: __init__() takes exactly 1 argument (3 given)
===============================================================================
[ERROR]: twisted.web.test.test_tap.ServiceTests.test_phpProcessor

Traceback (most recent call last):
  File "/srv/bb-slave/Run/slave/reactors/Twisted/twisted/web/test/test_tap.py", line 90, in test_phpProcessor
    self.assertIsInstance(root.getChild("foo.php", None), PHPScript)
  File "/srv/bb-slave/Run/slave/reactors/Twisted/twisted/web/static.py", line 311, in getChild
    return resource.IResource(processor(fpath.path, self.registry))
exceptions.TypeError: __init__() takes exactly 1 argument (3 given)
-------------------------------------------------------------------------------

One other thing. In the class docstring for FilteredScript, I think the sentence about customizing filter can be a bit broader - ie, this works for things other than PHP. :)

Thanks!

comment:13 Changed 5 years ago by jesstess

(In [28177]) Make tap tests that check warnings be aware of the new PHPScript and
PHP3Script warnings.

refs #516

comment:14 Changed 5 years ago by jesstess

(In [28179]) FilteredScript is for more than PHP.

refs #516

comment:15 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

comment:16 Changed 5 years ago by exarkun

Hmm. I'm wondering about the warnings that get emitted by twistd -n web --path ... now. It'd be nice to avoid those.

comment:17 Changed 5 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to jesstess

See previous comment.

comment:18 Changed 5 years ago by jesstess

(In [28380]) Delay calling PHP3Script and PHPScript in tap.Options.opt_path to
avoid displaying deprecation warnings on something like twistd -n web
--path .

refs #516

comment:19 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

comment:20 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess
  1. test_tap.py has its copyright date bumped, but no other changes. This should be reverted.
  2. test_PHP3ScriptIsDeprecated and test_PHPScriptIsDeprecated use getattr. Instead, they can just do twcgi.PHP3Script and twcgi.PHPScript.
  3. 10.0 has sailed. Update the deprecation version numbers to 10.1.
  4. As long as you're reformatting the CGIScript class docstring, can you switch it away from the I ... style? Something like L{CGIScript} is a resource which runs child processes according to the CGI specification. would be nice.

That's all. Please merge when the above points are addressed. Thanks.

comment:21 Changed 5 years ago by jesstess

(In [28558]) Don't use getattr; we're up to 10.1.

refs #516

comment:22 Changed 5 years ago by jesstess

  • Resolution set to fixed
  • Status changed from new to closed

(In [28559]) Merge phpscript-deprecation-516

Author: jesstess
Reviewer: exarkun
Fixes: #516

twisted.web.twcgi.PHP3Script and twisted.web.twcgi.PHPScript are now
deprecated in favor of twisted.web.twcgi.FilterScript.

comment:23 Changed 4 years ago by <automation>

  • Owner jesstess deleted
Note: See TracTickets for help on using tickets.