On Sun, 30 Oct 2016 15:03:39 +0100
Tobias Geerinckx-Rice <me@tobias.gr> wrote:

> Julien,
> 
> On 30/10/16 13:08, Julien Lepiller wrote:
> > here is a patch to add php to guix.  
> 
> Excellent! I see you've broken into my machine (probably through PHP),
> stolen my bitrotting PHP 7 package and greatly improved it. Thanks!
> 
> An incomplete review:
> 
> > +                (chdir "ext")  
> [...]
> > +                (chdir ".."))))  
> 
> Try with-directory-excursion.
> 
> > +                "--enable-fpm" "-with-openssl"  
> 
> s/-with-openssl/--with-openssl/, although the option would seem
> unnecessary if the result is the same.
> 
> > +                ;"--with-snmp"  
> 
> Best add a comment explaining why this is unavailable, desirable, and,
> if possible, what's needed to fix it.
> 
> +        #:tests? #f))
> 
> There are tests, but many fail. This should be explained in a comment
> (or fixed ;-). I keep tests enabled on my machine because I hate PHP
> and like to hear it scream.
> 
> Bonus fun fact: catastrophic test failure is non-fatal and the thing
> installs fine.
> 
> > +    (synopsis "PHP programming language")
> > +    (description
> > +      "PHP is one of the most commonly used programming language
> > the web")  
> 
> s/language/languages/ and a missing full stop, but it would be nice to
> add even more. For example:
> 
>   PHP (PHP Hypertext Processor) is a server-side (CGI) scripting
>   language designed primarily for web development but is also used as
>   a general-purpose programming language.  PHP code may be embedded
> into HTML code, or it can be used in combination with various web
> template systems, web content management systems and web frameworks.
> 
> Thanks again for working on this!
> 
> Kind regards,
> 
> T G-R
> 

Here is a new patch that addresses your comments and Efraim's. Also I'm
pretty sure you're not supposed to end a description with a full stop.