Coverage remained the same at 52.298% when pulling 93061a9 on morozov:default-without-mcrypt into 581eb9c on phpbrew:master.

Coverage increased (+0.01%) to 52.309% when pulling 93061a9 on morozov:default-without-mcrypt into 581eb9c on phpbrew:master.

changing the default varients will also modify settings for other php versions.

I suggest we still keep mcrypt in the default value, and add sodium into default as well for php 7.2+.

at the same time, we can remove mcrypt for php 7.2+, and remove sodium for pho 7.1- in VariantBuilder

I see a few issues with this suggestion:

1. Adding sodium to the default list may be a breaking change for those who compile PHP 7.2 but don't have libsodium installed. It's just a bad UX if I need to install a library to satisfy a dependency of the extension I don't need. And if I do need it, I'll specify it explicitly (we should add the sodium variant, I'll file it separately).
2. Currently, the default variant is static. When you run phpbrew variants, its extensions are displayed as they are. If we start massaging the list at runtime depending on the PHP version, it may create a confusion that the list of eventually installed extensions is different from what was originally offered.

changing the default varients will also modify settings for other php versions.

I'd say it's the least of the evils. Just add +mcrypt if you need it.

In general, I'd say the default variant shouldn't contain any extensions which have external dependencies (including mcrypt and sodium). However, things like openssl make sense to be installed by default since without SSL PHP is barely usable.

Adding sodium to the default list may be a breaking change for those who compile PHP 7.2 but don't have libsodium installed.

It's why we need some changes in VariantBuilder

we should add the sodium variant, I'll file it separately

agreed. In fact I have already finished this, but I won't be available (to push / open PR) before next Monday.

If we start massaging the list at runtime depending on the PHP version, it may create a confusion that the list of eventually installed extensions is different from what was originally offered.

emm, I just suggest to remove those unsupported extension at runtime. I also want to point out that we will automaticlly build opcache if the target version is 5.6 or above.

let's put the default value aside, another issue is everything. if we don't modify the build setting during runtime, how will you deal with it?

It's why we need some changes in VariantBuilder

Do you want to check the dependencies of sodium at run-time and not include the extension if dependencies are missing?

I just suggest to remove those unsupported extension at runtime

I don't like the approach. The set of extensions included in a variant should be predictable. If I run phpbrew variants and chose the phpbrew install +$variant, I would expect the same set of extensions installed as I saw in the variant description.

Do you want to check the dependencies of sodium at run-time and not include the extension if dependencies are missing?

yeah, I checked the dependencies, but if I cannot find I will just append --with-sodium and let the autotool to deal with it, like what we do for curl or mcrypt.

I would expect the same set of extensions installed as I saw in the variant description.

So what's your solution for +everything?

yeah, I checked the dependencies, but if I cannot find I will just append --with-sodium and let the autotool to deal with it, like what we do for curl or mcrypt.

But that's the problem. If as an end user I don't use sodium, I'll have either to install its dependencies or disable it explicitly. Why force users to use (or deal with) it by default?

So what's your solution for +everything?

Didn't know it's a thing. What's the use case for it? I'd say users who use it ask for a trouble. As soon as we add a new variant (e.g. ldap recently), the users who use it will have to deal with new dependencies. Having both mcrypt and sodium at a time will make it unresolvable and unusable. But I still don't want to have any version-specific filtering of variants. My solution for it is to get rid of it.

So, here's my proposal:

1. Merge this.
2. Add the sodium variant.
3. Drop the everything variant.

@morozov sounds good, but how do we handle older php versions?

Can we provide different variant list for these php major versions?

@morozov sounds good, but how do we handle older php versions? Can we provide different variant list for these php major versions?

I don't get the question. Removal of mcrypt from +default and removal of +everything is agnostic of PHP versions. The fewer extensions we assume by default, the more stable the installer will be.

I'd still like to get this merged. @c9s please see my response above.

@morozov sure, but many legacy applications based on php5.x use mcrypt extension, if might break some CI build flow if we deprecated it from the default.

would be great if we can add a dynamic check to add the mycrpt variant back dynamically for php5.x versions when user install php5.x.

would be great if we can add a dynamic check to add the mycrpt variant back dynamically for php5.x versions when user install php5.x.

Please see above:

Currently, the default variant is static. When you run phpbrew variants, its extensions are displayed as they are. If we start massaging the list at runtime depending on the PHP version, it may create a confusion that the list of eventually installed extensions is different from what was originally offered.

Legacy doesn't mean unmaintained. For such apps, their maintainer can add +mcrypt to the configuration and keep using it.

@morozov understood. thanks for the explanation. approved.

Would be great if you can add a notice message when user uses +default variant. To let the users know the application changes since it's a backward incompatible change.

Just adding a warning when someone uses the default variant might be as non-informative as the one removed in #880. There will be a significant fraction of users who use the default variant and don't care about mcrypt.

A better approach would be to show the warning only if +default is chosen without +mcrypt or -mcrypt. This way, once they see the warning, they could suppress it by making an explicit choice — which is exactly what I'd want.

I tried to implement it in the InstallCommand as:

if (isset($variantInfo['enabled_variants']['default'])
    && !isset($variantInfo['enabled_variants']['mcrypt'])
    && !isset($variantInfo['disabled_variants']['mcrypt'])) {
    $this->logger->warn("Deprecation messate... Please enable or disable mcrypt explicitly");
}

Unfortunately it didn't work as I expected and just added --without-mcrypt to the build arguments.

I think it's fine if we just highlight the change in the release notes. We've already spent more time and effort on this issue than it deserves.