Infer default PHP version from composer.json

ostrolucky commented 1 year ago

I love the idea of inferring default extensions to install from composer.json. I would like to have the same functionality for inferring default PHP version itself as well. At the moment, it's always defaulting to PHP 8.1, no matter what is in composer.json. I suggest to parse require.php from composer.json and detect minimum version from there. My initial attempt is

builtins.replaceStrings ["."] [""] (
  builtins.head (
    builtins.match ".*([[:digit:]]\.[[:digit:]])"
    (
      builtins.fromJSON (
        builtins.readFile "${builtins.getEnv "PWD"}/composer.json"
      )
    )
    .require
    .php
  )
)

but this ought to be improved, because this doesn't handle values like ^7.4 || ^8.0

drupol commented 1 year ago

Hi !

I had many thoughts about that feature already... But I like the flexibility that the current situation offers.

Let's say we implements your proposal, what would you suggest when the user wants to use the upper version instead of the lower one ?

ostrolucky commented 1 year ago

If users wants default, inferred from composer.json, they use nix shell github:loophp/nix-shell --impure. If they want specific one, they use nix shell github:loophp/nix-shell#php82 --impure

drupol commented 1 year ago

Having the composer.json if you want a file to be read by Nix, then the --impure flag needs to be added. Without that flag, Nix won't be able to read it at all, so I guess it boils down to:

nix shell github:loophp/nix-shell#env-php82

or

nix shell github:loophp/nix-shell#env-php82 --impure

Right?

ostrolucky commented 1 year ago

Behaviour for #*php* shells shouldn't be affected, only for #default. In that case yeah only when --impure flag is used it will read it from composer.json, otherwise it can keep the current default (php8.1)

drupol commented 1 year ago

That's actually a super cool idea ! I didn't think about that!!

Would you like to submit a PR ?

ostrolucky commented 1 year ago

Yeah I'll see how I am with time but it should be doable in foreseeable future :)

drupol commented 1 year ago

Good, looking forward to it !

drupol commented 1 year ago

I updated the codebase these last days, implementing that feature should definitely be easy.

Basically it boils down to parsing the PHP version in composer.json and assigning it to the new defaultPhpVersion variable.

drupol commented 1 year ago

I found this regex to work pretty fine: https://regex101.com/r/NWCilZ/1

But since Nix is using C++ std::regex, which doesn't support most extensions, we need to rewrite it accordingly.

RyanPrussin commented 3 months ago

I love the idea of inferring the version of PHP from composer.json or composer.lock, so I've been thinking about this a little bit trying to determine the optimal way to do this.

After thinking about it, I'm not sure require.php is the ideal field to check

Reasoning

We have to deal with all the extra symbols and take into account logical differences they are meant imply (~ vs ^ vs > vs < vs || vs etc.). I think doing this accurately across all use cases is probably harder than it initially appears to be
- For example, composer require php "^8.1 || <5.4 || ~7.4.1" is a valid command that uses a confusing ordering of version possibilities while intermixing varying symbol usages. If I understand correctly, in this use-case we'd have to write code that picks the middle value and then knows to pick the first version before 5.4, so 5.3.x if we go with the minimum version
The require.php field is often out-of-date compared to the actual version of PHP being used, as most frequently it will only get updated on major version updates, being generally ignored when minor + patch updates are made on the server
Even after all of those checks, this field is (surprisingly) not a mandatory field, so it may not always be present

An Alternative Idea

We could use the platform.php field in composer.json (which translates to platform-overrides.php in composer.lock). Like require.php, it is also not a guaranteed field, but I think it might be a better fit for the following reasons:

When it exists, it is meant to describe the version of PHP on the main production server that runs the code
It exists to enforce that version at the composer-level via version constraints, so when it is set it will give you an error and block you if you try to install a version of a package that does not support that specific platform version of PHP -- meaning all composer packages installed in composer.lock MUST allow this version of PHP in its version constraints
It does not support any special symbols, so it should be MUCH more straightforward to implement

Basic Usage for Alternative:

Note: it's important to use the composer CLI commands to make the change, as it helps validate invalid values to rule them out.

Set `platform.php` in `composer.json`:

// These are all valid use-cases we'd need to account for, and we should pick the max allowed version

// Translates to `8.3.8` for our purposes as of this writing
composer config platform.php 8

// Translates to `8.1.29` for our purposes as of this writing
composer config platform.php 8.1

// Translates to `8.1.1` for our purposes as of this writing
composer config platform.php 8.1.1

Propagate the value of `platform.php` in `composer.json` to `composer.lock` as `platform-overrides.php`:

composer update --lock

Conclusion

In my opinion, we'd probably want to check composer.lock for platform-overrides.php first, and only if that doesn't exist would we perhaps want to fallback to composer.json and the platform.php version, for the following reasons:

composer config platform.php "" is a valid command, but on updating the lock file we get an error because an empty string isn't allowed in platform-overrides.php. This means the composer.lock setting has stricter validation, and is therefore more likely to be safe
All composer packages listed in composer.lock must be compatible with the platform-overrides.php field if it exists; I don't believe that is necessarily true for platform.php in composer.json

It's just an idea; I'm curious what you think! 😄

drupol commented 3 months ago

I think we should tackle the issue in another way. For example, what I had in mind a couple of months ago was to create some kind of Composer plugin that would just return true or false based on the PHP version we (nix) provide.

For example, we could build a simple command line tool that would be working as such:

cd <directory/containing/composer.lock>
composer check-php <path-to-php-binary>

or

cd <directory/containing/composer.lock>
composer check-php-version 8.1.22

Then composer would tell you if that PHP version is valid for that specific composer.json file. We could do that for all the PHP versions in Nix and take the first one that returns true. I definitely think it could be a fun project to do.

We could even think further, I don't think creating a Composer plugin is even required. We could generate a composer.json from a template file, replacing dynamically the PHP version in it, and then send it to composer install (or similar).

{
    "require": {
        "PHP": "^8.1"
    }
}

If it fails, that means the PHP version in use is not valid, and we must test with the next available one.

WDYT of the idea?

loophp / nix-shell