[packagist] api v2 support

[yookoala]

Rebased #6508 on the current master branch. An attempt to fix #7020.

I have a packagist v2 only package (this one). The v1 API doesn't really show any information about my package. I'd want to fix this so badges for my package works normally. Please let me know what I need to do to get this merged.

---

Update: Seems the badge for my package was affected by the reduced update rate of v1 Packagist API. It works for me now. But I'd still like to contribute and fix this for long run.

[shields-ci]

	Messages
📖	✨ Thanks for your contribution to Shields, @yookoala!

Generated by 🚫 dangerJS against 54d5604

[chris48s]

Thanks for picking this up. #6508 was most of the way there and it would be good to get this over the line but it had fallen off the radar.

It has been quite a while since I looked at #6508 but having got back into it and looked over this, I've pushed a few more commits with my suggested changes to get this over the line.

Maybe another maintainer could give this a once over so we can get this one resolved.

---

In response to #6508 (comment) (I'll reply here to keep the conversation in this thread rather than split across two PRs):

The logic in packagist-version.service.js should only be dealing with tagged releases and the code doesn't call the ~dev endpoint, so we don't need the code for dealing with branch aliases.

[yookoala]

In response to #6508 (comment) (I'll reply here to keep the conversation in this thread rather than split across two PRs):

The logic in packagist-version.service.js should only be dealing with tagged releases and the code doesn't call the ~dev endpoint, so we don't need the code for dealing with branch aliases.

I think we need some clarification before carrying on the discussion.

If I read the code correct:

1. PackagistVersion never referenced BasePackagistService.fetchDev. I suppose that means it's not calling the ~dev.json endpoint, correct?
2. Packagist.transform seems to be map all the version attribute (i.e. the git tag) from the packages array items. Since I suppose it is using the ordinary p2 endpoint, the returned packages are all tagged commits, correct?
3. If includePrereleases is false, the tag strings are filtered by the function isStable() here from php-version.js is checking the pattern of the tag. If the tag ends with nothing (modifierLevel 3) or a patch* (modifierLevel 4), then the tag is consider a stable version tag. If the tag ends with alpha*, beta*, dev* or anything else, the provided version string is considered "not stable". This seems logical to me.
4. As mentioned in (2), no matter if includePrereleases is true or false, the filter target seems to be only the tagged release. And as described in (3), the Packagist.transform method seems to be just checking if the tag string pattern matches that of release versions. If both observations are correct, then nothing here involves development branch.

The logics here looks like it wants to fulfil the contract of the method handle, which has an includePrereleases parameter. The approach taken to fulfil that contract seems reasonable, too.

[chris48s]

If I read the code correct:

1. PackagistVersion never referenced BasePackagistService.fetchDev. I suppose that means it's not calling the ~dev.json endpoint, correct?
2. Packagist.transform seems to be map all the version attribute (i.e. the git tag) from the packages array items. Since I suppose it is using the ordinary p2 endpoint, the returned packages are all tagged commits, correct?
3. If includePrereleases is false, the tag strings are filtered by the function isStable() here from php-version.js is checking the pattern of the tag. If the tag ends with nothing (modifierLevel 3) or a patch* (modifierLevel 4), then the tag is consider a stable version tag. If the tag ends with alpha*, beta*, dev* or anything else, the provided version string is considered "not stable". This seems logical to me.
4. As mentioned in (2), no matter if includePrereleases is true or false, the filter target seems to be only the tagged release. And as described in (3), the Packagist.transform method seems to be just checking if the tag string pattern matches that of release versions. If both observations are correct, then nothing here involves development branch.

Yes I'd agree with that summary 👍

[yookoala]

@chris48s: I've modified BasePackagistService.findLatestVersion to accept an optional includePrereleases parameter. Then I simplified the logics in PackagistVersion.handle to use BasePackagistService.findLatestVersion instead of its own transform method.

Also did some rename and documentation of the existing methods.

Please check if this is what you asked for.

[lgtm-com]

This pull request introduces 1 alert when merging d0127c4 into a1885cd - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

[Seldaek]

I did some review on the behaviour of Packagist, the default page of a package seems shows:

1. The latest stable version. If not,
2. The latest version on the default branch.

Yes that seems accurate, per https://github.com/composer/packagist/blob/7493c36087514fbe269bc623edba9b8d6d1c96eb/src/Controller/PackageController.php#L570-L583 and https://github.com/composer/packagist/blob/7493c36087514fbe269bc623edba9b8d6d1c96eb/src/Entity/Package.php#L871-L874

The one slight inaccuracy is if the default branch is numeric and for example 2.x but you have a 3.x branch, then I believe it would still show 3.x-dev even tho that's not the default branch, but that only happens if you have no tagged version which is very rare anyway.

[yookoala]

Yes that seems accurate, per https://github.com/composer/packagist/blob/7493c36087514fbe269bc623edba9b8d6d1c96eb/src/Controller/PackageController.php#L570-L583 and https://github.com/composer/packagist/blob/7493c36087514fbe269bc623edba9b8d6d1c96eb/src/Entity/Package.php#L871-L874

The one slight inaccuracy is if the default branch is numeric and for example 2.x but you have a 3.x branch, then I believe it would still show 3.x-dev even tho that's not the default branch, but that only happens if you have no tagged version which is very rare anyway.

@Seldaek: So the default display would show:

1. The commit tagged with the most advanced stable semver (like v1.0.1 or 2.0.2). If not found, then
2. The commit tagged with the most advanced unstable semver (like v3.0.0-alpha1, v3.0.0-beta1 or v3.0.0-rc1). If not found, then
3. The default branch, if it is not named like a semver (like master or main, converts to dev-* in packagist display). If not found, then
4. The branch named with the most advanced semver-ish name (like 2.x or 1.x, converts to *-dev in packagist display).

Am I correct?

[chris48s]

I've kind of lost track of where we are with this PR but I'd really like to try and get this work landed one way or another.

The branch has been force-pushed a bunch of times so I've totally lost the thread of what changed since last time I reviewed it.

@yookoala - are you waiting on us to review this, or are we waiting for you to update it?

[yookoala]

I believe I made all changes that are asked by you and other reviewers. Please kindly check if any other amendment you might want me to do.

[chris48s]

Just having a cursory look at the build it is failing with InvalidService: Expected /usr/src/app/services/packagist/packagist-license.service.js to export a service or a collection of services; one of them was license not found. Can you have a look at this and make sure what is on this branch is definitely ready for review and I will try and find some time to get back into it next week.

[yookoala]

Just having a cursory look at the build it is failing with InvalidService: Expected /usr/src/app/services/packagist/packagist-license.service.js to export a service or a collection of services; one of them was license not found. Can you have a look at this and make sure what is on this branch is definitely ready for review and I will try and find some time to get back into it next week.

Should be fixed now. I've also re-run npm run test:core -- --only="packagist". Every tests are passed.

Please let me know what to check next, if any. Thanks.

[chris48s]

Thanks for your continued work on this but I tried re-running the build with the latest changes. There are still failing tests:

PackagistLicense should return the license of the most recent release
should return the license of the most recent release
/home/circleci/project/services/packagist/packagist-license.spec.js

TypeError: PackagistLicense.prototype.getLicense is not a function
    at Context.<anonymous> (file:///home/circleci/project/services/packagist/packagist-license.spec.js:30:39)
    at processImmediate (node:internal/timers:464:21)

PackagistLicense should return the license of the most recent stable release
should return the license of the most recent stable release
/home/circleci/project/services/packagist/packagist-license.spec.js

TypeError: PackagistLicense.prototype.getLicense is not a function
    at Context.<anonymous> (file:///home/circleci/project/services/packagist/packagist-license.spec.js:55:39)
    at processImmediate (node:internal/timers:464:21)

PackagistLicense should return the license of the most recent pre-release if no stable releases
should return the license of the most recent pre-release if no stable releases
/home/circleci/project/services/packagist/packagist-license.spec.js

TypeError: PackagistLicense.prototype.getLicense is not a function
    at Context.<anonymous> (file:///home/circleci/project/services/packagist/packagist-license.spec.js:80:39)
    at processImmediate (node:internal/timers:464:21)

PackagistLicense should throw NotFound when license key not in response
should throw NotFound when license key not in response
/home/circleci/project/services/packagist/packagist-license.spec.js

AssertionError: expected [Function] to throw 'NotFound' but 'TypeError: PackagistLicense.prototype…' was thrown
    at Context.<anonymous> (file:///home/circleci/project/services/packagist/packagist-license.spec.js:104:16)
    at processImmediate (node:internal/timers:464:21)

PackagistPhpVersion should return PHP version for the default branch
should return PHP version for the default branch
/home/circleci/project/services/packagist/packagist-php-version.spec.js

AssertionError: expected '^8.1' to equal '^7.4 || 8'
    at Context.<anonymous> (file:///home/circleci/project/services/packagist/packagist-php-version.spec.js:124:13)
    at processImmediate (node:internal/timers:464:21)

It seemed like we were close to getting this merged at one point. I can see you are trying to help, but it also feels like we're going round in circles a bit here. I am aware that the fact builds are not running on this branch for some reason is making it harder for both of us. I'm also finding the changes from e5b31cd onwards quite hard to understand. There is a lot of tangential refactoring mixed in with functional changes and I'm finding it difficult to follow the thread of where the behaviour of the code is changing intentionally, and where it is changing accidentally. I think you are too tbh - there are several points where this PR has been "ready" with tests still failing.

This is the second time someone has tried to pick up this issue and I'd really like to try and get #7020 closed. I'd like this not to fall through the cracks again. Here's an idea for a way forward:

If we take the code on this branch up to b164565 , is what's there fundamentally broken? i.e: if we merge the commit range 2a87125 - b164565 to master, are we actively introducing bugs? If the answer to that is no, I'm going to suggest we pick those commits to a new branch, merge that and then if you have some additional functional improvements or refactoring PRs you want to make to the packagist suite of badges you can make those changes in one or more follow up PRs which are more clearly scoped. This will be easier for both of us to reason about (and hopefully the builds will run automatically on a new PR). I accept that it is not always possible to completely decouple refactoring from functional changes and there are improvements that can be made to this code, but if we can break this down into smaller clearer objectives rather than trying to boil the ocean in one PR we're much more likely to achieve success.

[yookoala]

I'd agree. I was trying to improve the functionality and test coverage after b16465.

Also, what is the command to run all tests for packagist?

[calebcartwright]

Also, what is the command to run all tests for packagist?

npm run test:services -- --only=packagist

[chris48s]

Also, what is the command to run all tests for packagist?

npm run test:services -- --only=packagist

Sort of.

There are two types of tests relevant to this PR:

Unit tests

These are in files ending .spec.js and we run all of them when we run npm test (you can actually run npm run test:core to run these if you want - this is faster because it skips the frontend tests and the linter checks). The unit tests are quick to run and don't call out to any external services. These generally test quite specific functionality.

Service tests

These are in files ending .tester.js. These are a type of integration test. They're a bit higher level and they actually call out to external services. This is unusual practice in testing but we do this because one of the purposes of service tests is to notify us if a badge has broken due to an upstream API change. Because these tests call out to external APIs they are slower to run and can sometimes be flaky which is why we don't run them all on every PR or when we call npm test (but we do have a scheduled job that runs the full suite of service tests once a day). To run the packagist service tests locally you run npm run test:services -- --only=packagist (as @calebcartwright rightly says). Maybe one of the things that might have happened is you're modifying the unit tests (.spec.js files) but then running the service tests with npm run test:services -- --only=packagist and seeing them pass, or vice versa?

Hopefully that helps.

---

In any case, I have pulled out 2a87125 - b164565 into another PR over at #7681 . Lets get that one merged first. I will close this PR. Then if you want to do some follow-up PRs with functional tweaks, additional testing or structural improvements incorporating some of the concepts from this branch then we can look at them. Splitting that into a few smaller PRs with a clear objective would be really helpful.

[calebcartwright]

Yeah good shout. Was largely doing a drive by response from the notification inbox and assumed we were talking about service tests; missed the context of the failing tests in the core suite