-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat(composer): filter releases using php constraint #19851
Conversation
@@ -415,6 +416,60 @@ export async function getPkgReleases( | |||
version.matches(constraintValue, releaseConstraint) | |||
); | |||
}); | |||
} else { | |||
if (constraintName === 'php') { |
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.
Ideally don't hardcode this so that it can be reused for other ecosystems in future without a rewrite (especially python and node)
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.
While skimming through the code yesterday I thought the filtering could be done directly in the packagist lookup. Was I wrong or did you out it here to be potentially reusable on purpose?
UPDATE: nevermind, understood it I think
const phpVersions = ( | ||
await getPkgReleases(lookupConfig) | ||
)?.releases.map((release) => release.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 suggest that this is done once per run, for optimization reasons
if (!phpVersions) { | ||
logger.warn('Could not fetch php releases for compatibility check'); | ||
return null; | ||
} |
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.
This is a huge problem if it ever happens, not sure whether to throw an ExternalHostError or not
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.
You can cache this right? Because having to look this up for every single PHP run sounds wasteful. And if it happens, blow out and try again later?
const matchingVersions = phpVersions.filter((version) => | ||
composerVersioning.matches(version, constraintValue) | ||
); |
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.
This should also be done once per run per constraint
const releaseMatchingVersions = phpVersions.filter( | ||
(version) => | ||
composerVersioning.matches(version, releaseConstraint) | ||
); |
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.
Should also be calculated once per run per constraint
const isMatch = matchingVersions.every((version) => | ||
releaseMatchingVersions.includes(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.
This could also be cached per run
👍 this also closes #2355 right? Because AFAIK there was no way to do this via packagist API. |
Yeah, they're kind of duplicates |
# Conflicts: # lib/modules/datasource/packagist/index.ts # lib/modules/manager/composer/extract.ts
@@ -14,6 +14,7 @@ export const ComposerRelease = z.object({ | |||
.transform((x) => x.url) | |||
.nullable() | |||
.catch(null), | |||
require: z.object({ php: z.string() }).nullable().catch(null), |
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.
require: z.object({ php: z.string() }).nullable().catch(null), | |
require: | |
.object({ | |
php: z.string(), | |
}) | |
.transform((x) => x.php) | |
.nullable() | |
.catch(null), |
@@ -69,6 +71,12 @@ export function parsePackagesResponses( | |||
dep.releaseTimestamp = composerRelease.time; | |||
} | |||
|
|||
phpConstraint ??= composerRelease.require?.php; |
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.
phpConstraint ??= composerRelease.require?.php; | |
phpConstraint ??= composerRelease.require; |
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.
Since we care for just single field inside nested structure, we transform it up for simpler retrieval.
🎉 This issue has been resolved in version 35.7.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Changes
extractedConstraints
to composer.extractconstraints
to packagist.getPkgReleasesphp
releasesContext
Closes #18715
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: