Bug#495542: RFS: libcas-php
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.
> debian/control:
> > Depends: ${misc:Depends}, php5, php5-curl, php-db
>
> Does it really need a web SAPI?
Uh, SAPI ?
> 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 suppose most of the use cases would be satisfied as is... unless in
CGI mode, where php5-cgi would be ORed too ?
> debian/rules:
> What about cleaning it up?
>
Sure.
> debian/copyright:
> > Upstream Author:
> >
> > Pascal Aubry
>
> What about also displaying his email address?
Sure.
>
> > License: (stated at : http://www.ja-sig.org/wiki/display/CASC/phpCAS)
> what about docs/README?
>
Sure... and it appears to be missing from the package, btw :( Fixing
that.
> > - In case of jurisdiction dispute, the French law is authoritative.
> let's see what ftpmasters say about this.
OK.
>
> debian/dirs:
> empty? delete it
Yup.
>
> debian/docs:
> ditto
No longer, added upstream README.
>
> 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.
>
> 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 phpcas-devel@esup-portail.org 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 ?
> >
> > 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,
--
Olivier BERGER <olivier.berger@it-sudparis.eu>
http://www-public.it-sudparis.eu/~berger_o/ - OpenPGP-Id: 1024D/6B829EEC
Ingénieur Recherche - Dept INF
Institut TELECOM, SudParis (http://www.it-sudparis.eu/), Evry (France)
Reply to: