-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Conversation
@@ -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'])) { |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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": "*"
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-
).
fbc22d5
to
119a7de
Compare
119a7de
to
07f4a47
Compare
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