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

VersionConstraintParser does not support logical OR represented by single pipe #28

Closed
llaville opened this issue Feb 6, 2022 · 6 comments

Comments

@llaville
Copy link
Contributor

llaville commented Feb 6, 2022

Hello,

Context
I've wrote a solution that may be integrated in phar-io/manifest that is able to write a manifest.xml from parsing any composer.json file.

Issue
I've noticed that the VersionConstraintParser does not support logicial OR syntax represented by a single pipe.
This syntax is still used by lot of vendor / package.
See https://getcomposer.org/doc/articles/versions.md#version-range

Recommandation
I propose to add support of single pipe support for logical OR.
See https://github.com/phar-io/version/blob/3.1.0/src/VersionConstraintParser.php#L17
And https://github.com/phar-io/version/blob/3.1.0/src/VersionConstraintParser.php#L61

Expected behaviour
Prints "Phar.io version constraint: ^7.4|^8.0"

Actual behiavour
Raises Fatal error: Uncaught PharIo\Version\UnsupportedVersionConstraintException: Version constraint ^7.4|^8.0 is not supported.

Script to reproduce issue

<?php
use PharIo\Version\VersionConstraintParser;

$parser = new VersionConstraintParser();

$constraint = '^7.4|^8.0';
$versionConstraint = $parser->parse( $constraint );
var_dump('Phar.io version constraint: ' . $versionConstraint->asString());
@llaville
Copy link
Contributor Author

llaville commented Feb 6, 2022

Compare with Composer that is able to handle both syntaxes

<?php

use Composer\Semver\Semver;

$version = '8.1.0';

foreach (['^7.4|^8.0', '^7.4||^8.0'] as $constraint) {
    $res = Semver::satisfies($version, $constraint);
    if ($res) {
        var_dump("$version satisfy $constraint");
    }
}

// string(23) "8.1.0 satisfy ^7.4|^8.0"
// string(24) "8.1.0 satisfy ^7.4||^8.0"

@theseer
Copy link
Member

theseer commented Feb 6, 2022

I've wrote a solution that may be integrated in phar-io/manifest that is able to write a manifest.xml from parsing any composer.json file.

While that's off topic for this issue, I'd like to add an imporant side note here: I do like that idea, but please keep in mind that you would need to check the composer.lock as the idea of the manifest is not to have a version constraint but the exact actually bundled / installed version.

But let's look at your actual issue :)

Use || or |

I see your point but I'd like to point out that even composer claims the use of | is deprecated and only supported for BC reasons. I'm thus unsure if we should add it.
If you feel like providing a PR though, I might get persuaded ,)

@llaville
Copy link
Contributor Author

llaville commented Feb 7, 2022

Use || or |

I see your point but I'd like to point out that even composer claims the use of | is deprecated and only supported for BC reasons. I'm thus unsure if we should add it. If you feel like providing a PR though, I might get persuaded ,)

👍 A PR will follow

@llaville
Copy link
Contributor Author

llaville commented Feb 7, 2022

In PR provided, I've just added one complex logical or that combines both single and double pipe, to demonstrate the fix. Hope It's enough for you. Tell me if it's not !

@theseer
Copy link
Member

theseer commented Feb 7, 2022

Looks good on first glance. Will try to have an in depth look later and if everything is fine make a release with it included.

@theseer
Copy link
Member

theseer commented Feb 7, 2022

Done.

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

2 participants