@GrahamcOfBorg test nextcloud

I don't think we need an option to overwrite the php version. A list with extensions to enable would be much easier to use and less likely to break things. I am pretty sure Nextcloud does not support php80 yet or any much older version.

Both options seem fine to me.

This flexibility seems worthwhile to me. This option allows users to create a minimal php package as well as an added bonus.

I tested this successfully on my server. I enabled the bz2 and pdlib extensions with which I could install and enable https://apps.nextcloud.com/apps/facerecognition. Thanks!

Pulling in @etu and @talyz for opinions and review ❤️

This flexibility seems worthwhile to me.

and makes it harder to understand and way easier to do something breaking.

Please compare the size of those two expressions:

pkgs.php74.withExtensions ({ enabled, all }: enabled ++ [ all.pdlib all.bz2 ])

[ bz2 pdlib ]

Also the first is pinning php to 74 which is not ideal.

This option allows users to create a minimal php package as well as an added bonus.

Nextcloud and minimal is a good joke. I don't think you can even create a more minimal php package set with the standard installation.

This flexibility seems worthwhile to me.

and makes it harder to understand and way easier to do something breaking.

Please compare the size of those two expressions:

pkgs.php74.withExtensions ({ enabled, all }: enabled ++ [ all.pdlib all.bz2 ])

[ bz2 pdlib ]

I'm afraid it won't be that easy. bz2 and pdlib are not in scope, so it will have to be a pattern like this at least:

all: [ all.bz2 all.pdlib ]

To match the existing conventions, we could also write it as:

{ enabled, all }: enabled ++ [ all.pdlib all.bz2 ]

The latter pattern also allows us to disable certain extensions, which may be what the users wants (although I didn't have such a use case myself yet).

Also the first is pinning php to 74 which is not ideal.

That's true. This is a real advantage of only selecting extensions instead of making the whole package.

I agree with @SuperSandro2000 here: as maintainers we have to take care that a PHP we ship for Nextcloud works for the base module. It should be possible to use additional modules if needed and config can be written in the module itself.

Then the best option is probably a pattern like all: [ all.bz2 all.pdlib ] because this also rules out the possibility of accidentally disabling extensions needed by nextcloud

This would be the easiest:

pkgs.php74.withExtensions ({ enabled, all }: enabled ++ with all; [ extraPackages ])

extraPackages = [ bz2 pdlib ]

The latter pattern also allows us to disable certain extensions, which may be what the users wants

If we can't think about a usecase right now we should ignore it. If someone has a really good argument to add it we can always revisit it.

Also the first is pinning php to 74 which is not ideal.

You can drop the 74 to always use the latest version, i.e. just write

php.withExtensions ({ enabled, all }: enabled ++ [ all.pdlib all.bz2 ])

instead.

I do however agree that, if it's unnecessary to configure the version of PHP used, it would be better to only allow additional extensions to be specified.

This would be the easiest:

pkgs.php74.withExtensions ({ enabled, all }: enabled ++ with all; [ extraPackages ])

extraPackages = [ bz2 pdlib ]

That won't work, though; you'll get an undefined variable error when nix is trying to evaluate bz2 or pdlib, since they're not in scope. They have to be passed as an argument to a function; i.e. the argument to the option will have to be a lambda with one argument, the set of valid extensions.

@aanderse What do you think is the best way forward?

@turion - I think the best way forward is to add a phpPackage option but a room full of people, all smarter than myself, don't think this is the best way forward... so my opinion is wrong. I'm OK with that 😄

I don't use the nextcloud module so my participation ends here. I think it is pretty straight forward to implement any of the above suggestions. I'd just ask that when someone makes a PR they include db19515 because it removed duplicate generated configuration 👍

Ok, thanks for your work :) I'll make a new PR based on this.

Some context: There was a commit 2 years ago, going like this:

nixos/nextcloud: don't make phpPackages configurable

It needs to match the version in phpfm which is hard coded.
So there is no point in being able to change it.

This is why the package should not be configurable.

Yes, this made perfect sense before some of our superstars (@etu and @talyz) rewrote the entire php ecosystem to handle these shortcomings. The reasoning from that comment is no longer valid. Other reasons seem to be why people don't want this to happen now 👍

Ok, makes sense, thanks!