Hi, [No need to CC me as long as -mentors is]
Olivier Berger wrote: > Hi. > > Thanks for your review. I'll respond bellow. > > Le lundi 17 novembre 2008 à 15:20 -0600, Raphael Geissert a écrit : >> Olivier Berger wrote: >> [...] >> > >> > The corresponding ITP is found at >> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=495542 >> > >> > Note that this package would allow the dependency of a number of >> > packages such as moodle and glpi on that package instead of shipping a >> > copy of phpCAS (http://packages.debian.org/sid/all/moodle/filelist and >> > http://packages.debian.org/sid/all/glpi/filelist). >> >> While you are at it, as your package also uses domxml-php4-php5.php why don't >> you also package it? (it would be better if the code was finally ported >> instead of using a wrapper, though). >> > > I can see a couple of reasons why packaging domxml-php4-php5.php may not > be needed : > * it's a one file library, so I'm not sure it's really a habit of > packaging individual files ? > * it may be included in Debian packages only by those which include a > copy of phpCAS (but confirmation would be welcome) > * It will probably be no longer needed in phpCAS when future releases > will be rewritten (as you suggest) to only support php5, thus using the > php5 DOM API instead of the old PHP4 one. But that's not something that > would be done only for Debian, and I'll wait until a decision is taken > upstream (although I may also again contribute to upstream development > for that). More details here : > http://www.ja-sig.org/issues/browse/PHPCAS-25 about a rewrite to get rid > of that wrapper. > > So until usptream phpCAS releases a new version which allows to get rid > of domxml-php4-php5.php, I'll stick with it in. Yeah, I forgot that all the matches I found of the dom lib were because of the embedded copy of phpCAS > >> debian/control: >> > Depends: ${misc:Depends}, php5, php5-curl, php-db >> >> Does it really need a web SAPI? > > Uh, SAPI ? SAPI: Server Application Programming Interface E.g. apache, apache2, cgi, cli, embedded, roxen, etc. > >> or can it be used with php5-cli? in the latter >> case add an ORed dependency on php5-cli in addition to php5. >> > > I have no clue. > Typical use of CAS is traditionally on web apps, and the protocol > involves redirections between the CAS server and clients, so I'm not so > sure a non web use is something interesting. I saw that, but CAS.php looked like a more general pourpose script, although I didn't examine it very well. > > I suppose most of the use cases would be satisfied as is... unless in > CGI mode, where php5-cgi would be ORed too ? No, php5-cgi is already covered by php5. >> >> debian/README.Debian: >> > This packaging needs testing as I'm not fully sure there ain't >> > regressions introduced by this upgrade of the DOM parsing library. >> >> Have you at lest tested the basic functionality of the library with the newer >> version of domxml-php4-php5.php or are you blindly packaging stuff? > > No I have tested it, but you never exactly know for corner cases like > error messages hard to test. > > Btw, my main motivation for packaging it is to use it on a production > server, so I'd better have it tested of course. > > In any case I preferred some explicit mention that it's not exactly > upstream's version, to let people know who's to blame in case of > problem. > > Will rephrase as : > > Differences from upstream version : > ----------------------------------- > > This package includes a Debian specific patch, to replace > domxml-php4-php5.php version 1.5.5 (shipped with version 1.0.1 > of > phpCAS) by version 1.20a of domxml-php4-to-php5.php, which is > now > under LGPL, making it DFSG-free. > > Best care was taken by the Debian packager to make sure there > ain't > regressions introduced by this upgrade of the DOM parsing > library. Please report to the Debian BTS if you experience > problems > related to that change. > Ok, looks better now. >> >> CAS.php: >> > define("CAS_PGT_STORAGE_FILE_DEFAULT_PATH",'/tmp'); >> .. >> > define("CAS_PGT_STORAGE_FILE_FORMAT_PLAIN",'plain'); >> >> Doesn't look good at all. > > Hmmm... I guess that needs to be fixed indeed. Thanks for spotting that. > >> >> CAS/client.php: >> > if ( $this->isProxy() ) { >> > // pass the callback url for CAS proxies >> > $validate_url .= >> > '&pgtUrl='.$this->getCallbackURL(); >> > } >> >> Improper sanitation of getCallbackURL which uses data like REQUEST_URI; and >> there's more about this, but I'm better going to test it and send the info to >> bugtraq. > > Feel free to notify me and/or [EMAIL PROTECTED] please (or have a > look at http://www.ja-sig.org/wiki/display/CASC/phpCAS+bug+reports . > >> >> What about also shipping the api docs? >> > > Well maybe in a separate -doc package, then ? IIRC it was pretty small, so there's no need, IMHO, to ship it in another package. > >> > >> > I would be glad if someone uploaded this package for me. >> > >> > Kind regards >> > Olivier Berger >> >> Cheers, >> - -- >> Raphael Geissert - Debian Maintainer >> www.debian.org - get.debian.net > > Thanks alot for the detailed review. > > Regards, Cheers, -- Raphael Geissert - Debian Maintainer www.debian.org - get.debian.net -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]