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

phpversion('frankenphp') should not have the leading v #788

Open
TimWolla opened this issue May 15, 2024 · 4 comments
Open

phpversion('frankenphp') should not have the leading v #788

TimWolla opened this issue May 15, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@TimWolla
Copy link
Contributor

What happened?

Compared to every other extension I checked (except for mysqlnd), FrankenPHP is the only one whose version number has a v prefix. This is inconsistent with the greater ecosystem and breaks naive version_compare() checks to verify a specific FrankenPHP version is used (or not used), because the v as a string orders before all numbers:

php > $versions = ['1.1.4', 'v1.1.5', '1.1.6'];
php > var_dump($versions);
array(3) {
  [0]=>
  string(5) "1.1.4"
  [1]=>
  string(6) "v1.1.5"
  [2]=>
  string(5) "1.1.6"
}
php > usort($versions, version_compare(...));
php > var_dump($versions);
array(3) {
  [0]=>
  string(6) "v1.1.5"
  [1]=>
  string(5) "1.1.4"
  [2]=>
  string(5) "1.1.6"
}

Here's an example list of extensions with their phpversion($ext) output, both bundled extensions as well as extensions from pecl (e.g. Imagick, or Redis):

                Core: 8.3.6
               ctype: 8.3.6
                curl: 8.3.6
                date: 8.3.6
                 dom: 20031129
                exif: 8.3.6
            fileinfo: 8.3.6
              filter: 8.3.6
          frankenphp: v1.1.4
                  gd: 8.3.6
                 gmp: 8.3.6
                hash: 8.3.6
               iconv: 8.3.6
             imagick: 3.7.0
                intl: 8.3.6
                json: 8.3.6
              libxml: 8.3.6
            mbstring: 8.3.6
              mysqli: 8.3.6
             mysqlnd: mysqlnd 8.3.6
             openssl: 8.3.6
                pcre: 8.3.6
                 PDO: 8.3.6
           pdo_mysql: 8.3.6
          pdo_sqlite: 8.3.6
                Phar: 8.3.6
               posix: 8.3.6
              random: 8.3.6
            readline: 8.3.6
               redis: 6.0.2
          Reflection: 8.3.6
             session: 8.3.6
           SimpleXML: 8.3.6
              sodium: 8.3.6
                 SPL: 8.3.6
             sqlite3: 8.3.6
            standard: 8.3.6
           tokenizer: 8.3.6
                 xml: 8.3.6
           xmlreader: 8.3.6
           xmlwriter: 8.3.6
        Zend OPcache: 8.3.6
                 zip: 1.22.3
                zlib: 8.3.6

I would recommend dropping the leading v for phpversion('frankenphp') for consistency.

Build Type

Docker (Debian Bookworm)

Worker Mode

No

Operating System

GNU/Linux

CPU Architecture

x86_64

PHP configuration

Not relevant.

Relevant log output

No response

@TimWolla TimWolla added the bug Something isn't working label May 15, 2024
@withinboredom
Copy link
Collaborator

I would think that as long as we're internally consistent with version numbers (whether there is a "v" or not a "v"), it will be fine as it doesn't make sense to compare versions with other packages.

I also believe that Frankenphp uses Semver, which php's version_compare doesn't really understand and usually starts with a v. In that case, you only need to check the major version to ensure backwards compatibility, and major.minor for feature compatibility.

Can you describe what you're trying to do and why? I think if we understood the use-case and why it is important, it would help a case on changing how things are versioned vs. just suggesting that things be versioned in a certain way.

@TimWolla
Copy link
Contributor Author

I also believe that Frankenphp uses Semver, which php's version_compare doesn't really understand and usually starts with a v. In that case, you only need to check the major version to ensure backwards compatibility, and major.minor for feature compatibility.

The version number as per Semver starts with a digit: https://semver.org/lang/de/#backusnaur-form-grammatik-f%C3%BCr-valide-semver-versionen

The v prefix is just a common convention to make tags in git easier to distinguish from branches (e.g. for autocompletion purposes), but it is not part of the version number.

I would think that as long as we're internally consistent with version numbers (whether there is a "v" or not a "v"), it will be fine as it doesn't make sense to compare versions with other packages.

The output of phpversion() is meant for programmatic consumption, it should return the raw version number. It's fine to use the v indicator in other places, e.g. frankenphp --version.

Can you describe what you're trying to do and why? I think if we understood the use-case and why it is important, it would help a case on changing how things are versioned vs. just suggesting that things be versioned in a certain way.

Note that I am not suggesting to change the versioning. I'm just suggesting to drop the prefix from the programmatic access.

As for your question: I don’t have a direct use case (yet), this was a random find and I wanted to raise it before folks stumble upon this when trying to version_compare() the version (e.g. to work around a FrankenPHP bug in a library).

@dunglas
Copy link
Owner

dunglas commented May 15, 2024

TBH, I wasn't even aware that it was possible to use phpversion() to get FrankenPHP's version and I've no idea from where this function gets the number. I agree that removing the v in this context makes sense for consistency with other packages.

@TimWolla
Copy link
Contributor Author

and I've no idea from where this function gets the number

It's taken from the zend_module_entry:

TOSTRING(FRANKENPHP_VERSION),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants