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

workaround to debian php version that does not use semver #142

Merged
merged 3 commits into from
May 12, 2018
Merged

workaround to debian php version that does not use semver #142

merged 3 commits into from
May 12, 2018

Conversation

eclipxe13
Copy link
Contributor

Debian is using a version that is breaking SemVer (example 7.0.29-1)
This is on PHP_VERSION constant, but as workaround the php version can be built from constants PHP_MAJOR_VERSION, PHP_MINOR_VERSION & PHP_RELEASE_VERSION

This PR uses this concatenation strategy on PharIo\Phive\CompatibilityService::canRun and \PharIo\Phive\Environment::getRuntimeVersion.

This would also fix #136
Related: phar-io/version#10

@MatthiasKuehneEllerhold
Copy link

PHP_VERSION_ID should have the correct PHP version (e. g. 70200 for 7.2.0)

@eclipxe13
Copy link
Contributor Author

It does, but the value passed to \PharIo\Version\Version::__construct is expected to be a SemVer string

@theseer
Copy link
Member

theseer commented May 9, 2018

I do not have to understand why Debian only breaks ONE of the potential version identifiers but inconsistently does not touch the others I presume.

Interesting approach. I'm inclined to merge it, but am wondering how RC builds are going to be affected with this modification? Will it drop it or does the the PHP_RELEASE_VERSION contain the RC-suffix?
Did anyone check that? :)

@eclipxe13
Copy link
Contributor Author

I see. If PHP_RELEASE_VERSION does not contains the rc-suffix this patch would break requirements that depends on it.

Don't know the answer, will verify tomorrow with a docker container.

@eclipxe13
Copy link
Contributor Author

According to http://php.net/manual/en/reserved.constants.php the PHP_RELEASE_VERSION does not contains the extra information. The PHP_VERSION is formed by major.minor.release[extra]

  • major: PHP_MAJOR_VERSION (integer)
  • minor: PHP_MINOR_VERSION (integer)
  • release: PHP_RELEASE_VERSION (integer)
  • extra: PHP_EXTRA_VERSION (string)

The example from documentation is PHP_VERSION => "5.2.7-extra" where PHP_MAJOR_VERSION => 5, PHP_MINOR_VERSION => 2, PHP_RELEASE_VERSION => 7 and PHP_EXTRA_VERSION => "-extra"

So, as far I can understand, PHP is not using semantic versioning officially. I search for a document or information but nothing. The closest to a standard is the documentation of version_compare.

If PHP is not following SemVer then asserting that PHP_VERSION is a valid SemVer string is not correct. Also Debian would not be doing wrong by breaking SemVer since SemVer is not required.

PHP encourage you to ensure that version_compare will not brake in release process

PHP also establish that comparing PHP versions should follow this specification from version_compare:
"any string not found in this list" < dev < alpha = a < beta = b < RC = rc < # < pl = p.
But using \PharIo\Version\Version will not allow <any string>, rc (lowercase), # or pl.

So, instead of the current concatenation strategy I would like to propose that try to create the Versionobject from PHP_VERSION and if it throws a InvalidVersionException then create the object based on the concatenation and after that do the comparison, something like

// ...
case $requirement instanceof PhpVersionRequirement: {
    $php = $requirement->getVersionConstraint();
    try {
        $phpversion = new Version(PHP_VERSION);
    } catch (InvalidVersionException $ex) {
        $phpversion = new Version(PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION . '.' . PHP_RELEASE_VERSION);
    }
    if (!$php->complies($phpversion)) {
        $issues[] = sprintf(
            'PHP Version %s required, but %s in use',
            $php,
            PHP_VERSION
        );
    }
    continue 2;
}
// ...

Other different change would be to create the class \PharIo\Version\PhpVersion and ensure that it can compare fine against other PhpVersion object or other Version object. I don't know if this is a good idea for phar-io/version package. Or maybe implement this in \PharIo\Phive\PhpVersion.

@eclipxe13
Copy link
Contributor Author

After quick confirmation from @SaraMG PHP does not follow SemVer. I'm changing my PR to this:

  • Revert change on \PharIo\Phive\Environment::getRuntimeVersion, its out of the scope of this workaround.
  • Inside PharIo\Phive\CompatibilityService::canRun try to create Version object based on PHP_VERSION, if it fails try to use version concatenating major, minor and release constants.

@theseer
Copy link
Member

theseer commented May 12, 2018

I'll merge this for now as a workaround.

In the longer run, this should be replaced by a dedicated PHPVersion class that is "relaxed" in terms of semver compliance. As that needs an updated version of phar-io/version, that'll come later.

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

Successfully merging this pull request may close these issues.

Error when installing phing (other packages were ok)
3 participants