Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infer default PHP version from composer.json #136

Open
ostrolucky opened this issue Mar 12, 2023 · 11 comments
Open

Infer default PHP version from composer.json #136

ostrolucky opened this issue Mar 12, 2023 · 11 comments

Comments

@ostrolucky
Copy link

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
Copy link
Contributor

drupol commented Mar 13, 2023

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
Copy link
Author

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
Copy link
Contributor

drupol commented Mar 13, 2023

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
Copy link
Author

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
Copy link
Contributor

drupol commented Mar 13, 2023

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

Would you like to submit a PR ?

@ostrolucky
Copy link
Author

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

@drupol
Copy link
Contributor

drupol commented Mar 13, 2023

Good, looking forward to it !

@drupol
Copy link
Contributor

drupol commented Mar 15, 2023

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
Copy link
Contributor

drupol commented Mar 18, 2023

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
Copy link

RyanPrussin commented Jun 23, 2024

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
Copy link
Contributor

drupol commented Jun 23, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants