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

ArrayLoader: handle none string values for version/version_normalized #10470

Merged
merged 4 commits into from Jan 21, 2022

Conversation

glaubinix
Copy link
Contributor

Asserting that Package::getVersion/Package::getPrettyVersion always return a string. Currently this can return bool/int/float/string.

Additionally catching cases where version is not an array/object which currently throws a TypeError : trim(): Argument #1 ($string) must be of type string, array given

@glaubinix glaubinix changed the title ArrayLoader: handle none string values ArrayLoader: handle none string values for version/version_normalized Jan 19, 2022
@@ -113,12 +113,12 @@ private function createObject(array $config, $class)
if (!isset($config['name'])) {
throw new \UnexpectedValueException('Unknown package has no name defined ('.json_encode($config).').');
}
if (!isset($config['version'])) {
if (!isset($config['version']) || !is_scalar($config['version'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would even use is_string here. A valid package metadata contains a strict here, not other scalars (same for version_normalized)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is a good idea. There are packages in popular repositories using int values as versions. This would essentially break those packages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which popular packages ?

Btw, for version_normalized, this should be doable: a normalized version cannot be represented as an int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on any package on packagist.org but on wpackagist. Hard to say how common this is in other repositories.

Further limiting version_normalized could potentially break existing setups. For instance, the composer.json below successfully completes a composer update with the latest Composer release:

{
    "repositories": [
        {"type": "package", "package": {
            "name": "smarty/smarty",
            "version": "3",
            "version_normalized": 3,
            "dist": {
                "url": "https://www.smarty.net/files/Smarty-3.1.7.zip",
                "type": "zip"
            }
        }}
    ],
    "require": {
        "smarty/smarty": "*"
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but I think as long as we just overwrite version_normalized with a wrong type and not error then this should be ok.

Copy link
Contributor

@stof stof Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then wpackagist should really be fixed.

Providing a version_normalized that is not actually the normalized version as computed by Composer will break things when comparing versions (for inline packages, it is recommended to omit it and let composer compute it, but for composer repositories like packagist.org that have lots of packages, pre-computing the normalized version server-side helps with performance). And a valid normalized version cannot be casted as int, as it always have 4 segments (or starts with dev-).

@Seldaek Seldaek added this to the 2.2 milestone Jan 21, 2022
@Seldaek Seldaek added the Bug label Jan 21, 2022
@Seldaek Seldaek merged commit 3b4afaa into composer:2.2 Jan 21, 2022
@glaubinix glaubinix deleted the f/package-version-non-string branch January 21, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants