Re: [PATCH] Add php

[Tobias Geerinckx-Rice <me@tobias.gr>]

[-- Attachment #1.1: Type: text/plain, Size: 1637 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 476 bytes --]