nee writes:

> Hello sorry about not replying for such a long time, and thank you for
> reviewing my patches again.
>
> Am 02.11.2017 um 20:17 schrieb Christopher Baines:
>>
>> I've now attached a system test for php-fpm, that sets it up with
>> nginx, creates a basic php file, and checks that it can be requested.
>
> That's great thanks for saving me a bunch of work with this. ;-)
>
>>
>> This seems to work, which I was a little surprised at, as I was
>> suspecting problems with the socket permissions.
>>
>> I had a look, and while the nginx workers in the test system are not
>> root, the nginx master process is, so maybe that allows it to work...
> I don't think that's the reason, because I remember it not working at
> first when I didn't have the permissions set.

Yep, my mistake. I didn't spot the changes to the nginx service.

>>> I renamed the default workers-log-file to php7-fpm.www.log as it is
>>> usually called. The php-fpm log-file is now called php7-fpm.log.
>>
>> What I think I meant to say here was, I'm not sure the php _version_ is
>> adding much to the log file name (rather than "I'm not sure the php is
>> adding").
>>
>> The php package version is used in a few places, and while I can
>> imagine this being consistent with other distributions, it does add a
>> bit of complexity to the default values in the service, and I'm not
>> sure what benefit it brings?
>>
> If users want to run multiple php versions, they only have to change the
> version in the php-package and pass that package along all the services.
>
> My perception of the php landscape was that the major releases aren't
> 100% reliably backwards compatible and some applications depend on older
> stable releases, so that it is not too uncommon to run multiple php
> versions the same system.
>
> Here is a quote from: https://wordpress.org/about/requirements/
>
> """
> Why do we support older versions?
>
> We strongly recommend the latest versions of PHP and MySQL, but we
> understand that this isn’t right for everyone, and that sometimes hosts
> can be slow or hesitant to upgrade their customers since upgrades to PHP
> and MySQL have historically broken applications.
> """
>
> An alternative could be to include the php-package hash in the socket
> name, but I'm not sure if that would work with nginx and it's currently
> missing reload when a system-reconfigure is done. Or services could be
> generally more isolated somehow.
>
> WDYT?

That sounds good to me. I don't know too much about this area though.

>>>>> +  (file          php-fpm-configuration-file ;#f | file-like
>>>>> +                 (default #f)))
>>
>> ...
>>
>>>>> +                       (service-extension account-service-type
>>>>> +                                          php-fpm-accounts)))
>>>>> +                (default-value (php-fpm-configuration))))
>>>>
>>>> Filling in the description (a relatively new field on the service type)
>>>> would be a great addition here.
>>>>
>>> Ah, yes I'm mostly using the web documentation and other services from
>>> web.scm as reference. Thanks for the update.
>>> What would be a good value for this field? I just used "The php-fpm
>>> service-type." for now.
>>
>> That is ok, but it could probably be more useful. I think ideally it
>> would describe more about what the service offers.
>>
>> Users might encounter this when searching for services for example:
>>
>> → guix system search php
>> name: php-fpm
>> location: gnu/services/web.scm:607:2
>> extends: shepherd-root activate account
>> description: The php-fpm service-type.
>> relevance: 5
>
> I ran `guix sysytem search *` and it seems most descriptions start with
> 'Run' or 'Provide' I changed it to:
>  "Run `php-fpm' to provide a fastcgi socket for calling php through a
> webserver."
>
>
>> I've attached a patch containing a couple of copy+paste fixes, and a
>> system test.
>>
>> It would be good to get your opinion on the system test, does it test
>> the right things?
>>
> The tests look good to me.
> I added another test that adds two numbers and checks for the result in
> the response to see if php actually did something with the file.
>
>> If you like the look of it, I'd suggest including those changes in the
>> commit that adds the service, and then sending the updated patches.
>> I've included a changelog in the commit message.
>>
> Here are the updated patches. Thank you for your tests!

Thanks for picking this up again. Unfortunately it's also take me a long
time to get back around to looking at this.

I've attached some changes that I thought would be good when I was
looking through this. To give a rough summary:

 - Minor improvements to the docs, either content, markup or formatting.
 - Removing trailing whitespace.
 - Removing the changes to the nginx-service, in favour of changing the
   default socket group.
 - Change some indentation to avoid long lines.

By changing the default socket group, the system test passes, even
without the changes to the nginx service. I think this is a bit better,
and while it's definately not perfect, I think it would be ok to merge
with this change.

To also try and move the first patch forward, I've submitted that within
#29629, with an additional patch to get other services using
version-major.

It would be good to get your thoughts on the changes in the attached
patch, and then if you could send an updated set of patches, that would
be great. As far as I remember, the changes to the nginx service were
the only thing I felt needed addressing before merging this.

Thanks,

Chris