frankenphp icon indicating copy to clipboard operation
frankenphp copied to clipboard

`phpversion('frankenphp')` should not have the leading `v`

Open TimWolla opened this issue 1 year ago • 4 comments

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 avatar May 15 '24 07:05 TimWolla

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.

withinboredom avatar May 15 '24 07:05 withinboredom

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).

TimWolla avatar May 15 '24 10:05 TimWolla

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.

dunglas avatar May 15 '24 11:05 dunglas

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

It's taken from the zend_module_entry:

https://github.com/dunglas/frankenphp/blob/469070ce8573fc2cd9453d2559d8b5d9d0fa93fb/frankenphp.c#L419

TimWolla avatar May 15 '24 11:05 TimWolla