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

Improvements to package downloads #327

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

hugopeek
Copy link
Contributor

What does it do ?

This ensures the latest package version is always downloaded if multiple versions are available. As far as I could test, the correct package is always on top of the list in the response, so I think using the first result is reliable.

I don't really know how that list is being sorted though, so just to be sure it also checks if the package name is present in the signature. This means it always returns 1 package, so I disabled the "(x) packages found" message in the console output.

I also noticed an incorrect variable name on line 195. Well.. PhpStorm did ;) That suggests that the direct match was never working in the first place and it always fell back on the query straightaway.

Fixing that seems to make it possible (again?) to define the exact package version in .gitify and prevent updating to a newer version when re-running package:install --all.

Why is it needed ?

My initial issue was that Redactor now offers 2 versions through the ModMore package provider and it always installs the last one in the array (which is the lower version). Now it installs Redactor 3 when using 'redactor' in config. Ideally, you could specify an older version in the config file if you don't want to upgrade to v3, but somehow for the ModMore provider I can't get this to work..

In addition, Gitify often starts installing incorrect and unrelated packages when the package could not be found. This is annoying and shouldn't be the case. It should look for 1 package only and return either the latest version (formit) or a specific version (formit-4.2.0-pl). Otherwise an error that no match was found.

Related issue(s)/PR(s)

#262

And for anyone also unable to install colorpicker with Gitify, that should work now too. There seems to be a glitch in that package with minor version not set.

@sebastian-marinescu
Copy link
Contributor

Very interesting 😃 I'll test that soon 👍

@Mark-H
Copy link
Member

Mark-H commented May 1, 2020

As far as I could test, the correct package is always on top of the list in the response, so I think using the first result is reliable.

This cannot be guaranteed. I recall there being an issue way back where a search for "formit" would return "formitfastpack" before formit itself, so I'm worried about reintroducing that.

Ideally, you could specify an older version in the config file if you don't want to upgrade to v3, but somehow for the ModMore provider I can't get this to work..

The modmore provider does limit what versions can be downloaded as well, so specifying an arbitrary version probably wont work. Only versions that are marked as downloadable on my end (which typically is the last patch version of each minor release up to a year ago, e.g. 2.4.0 and 3.0.1) can be downloaded.

@hugopeek
Copy link
Contributor Author

This one is confusing, also for me. But I think I see now what's going on. I'll try to add comments:

'signature' => (string) $foundPkg->signature
);

if ($foundPkg) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically the same fix as in #353, so I can take this out.

This is the first request that looks for the exact signature, including version number.

foreach ($foundPackages as $foundPkg) {
$packages[strtolower((string)$foundPkg->name)] = array(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is looking at the package name, not the signature. It uses that name to create a key in the packages array.

The problem I had with Redactor, is that there are several versions available through the ModMore package provider, but they all have the same name! Result: on every foreach loop, the Redactor key is overwritten by the next hit. And in this case, that's the oldest package version.

Here are the results without this fix:

Searching modmore.com for redactor...
Array
(
    [redactor] => Array
        (
            [name] => Redactor
            [version] => 2.4.0
            [location] => https://rest.modmore.com/package/download?api_key=123&database=mysql&revolution_version=Revolution-2.8.3-pl&http_host=localhost&package=1&version=redactor-2.4.0-pl
            [signature] => redactor-2.4.0-pl
        )

)

foreach ($foundPackages as $foundPkg) {
$packages[strtolower((string)$foundPkg->name)] = array(
$packageVersions[] = array(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And when you put them all in an array first:

Searching modmore.com for redactor...
Array
(
    [0] => Array
        (
            [name] => Redactor
            [version] => 3.1.0
            [location] => https://rest.modmore.com/package/download?api_key=123&database=mysql&revolution_version=Revolution-2.8.3-pl&http_host=localhost&package=1&version=redactor-3.1.0-pl
            [signature] => redactor-3.1.0-pl
        )

    [1] => Array
        (
            [name] => Redactor
            [version] => 3.0.2
            [location] => https://rest.modmore.com/package/download?api_key=123&database=mysql&revolution_version=Revolution-2.8.3-pl&http_host=localhost&package=1&version=redactor-3.0.2-pl
            [signature] => redactor-3.0.2-pl
        )

    [2] => Array
        (
            [name] => Redactor
            [version] => 2.4.0
            [location] => https://rest.modmore.com/package/download?api_key=123&database=mysql&revolution_version=Revolution-2.8.3-pl&http_host=localhost&package=1&version=redactor-2.4.0-pl
            [signature] => redactor-2.4.0-pl
        )

)

@hugopeek hugopeek force-pushed the package-install-latest branch from 6a30e45 to 0b64464 Compare October 13, 2021 11:36
Didn't realize this was merged already. Thanks! :)
@hugopeek hugopeek force-pushed the package-install-latest branch from 0b64464 to d68ec94 Compare October 13, 2021 11:50
@hugopeek
Copy link
Contributor Author

hugopeek commented Oct 13, 2021

Ok, I think I managed to sort it out. Roughly, from top to bottom:

Phase 1: exact match

  • Only the full signature is now considered an exact match (formit-4.2.6-pl).
  • For some mysterious reason, a different version is returned sometimes by the ModMore provider (searching for redactor-2.4.0-pl still gives me the latest version).
  • To work around this issue, a second attempt is made on name only, after which the result is checked for a matching signature.
  • If there's no matching signature, go to the next step.

Phase 2: query for name

This is where things got shaky before, because the response also contains other packages with a partial match on name. The full match was usually on top of the list, but not always... So:

  • In the first foreach, only accept an exact match on the name (names are always converted to lowercase now first).
  • In most cases, that's all it takes. The packages array is set with just 1 result and it will move on to processing.
  • In some cases though, there are multiple versions of a package available under the same name (there are currently 3 versions of Redactor available, for example). That's the original issue where this PR was aimed at. The foreach would simply loop through the 3 available Redactor versions, each time overwriting the 'redactor' key in the packages array. This resulted in the lowest available version being installed.
  • To work around this, a packageVersions array is created on the side, to collect multiple versions if present.
  • If there are multiple versions of the same package, the highest is selected and added to the packages array instead.
  • By now, we would almost certainly have a match.. But if that's not the case, then the original foreach loop is run, adding anything it can find to the packages array. So only if you make a typo (formi instead of formit), will it reach this far.

Phase 3: processing

This is still intact, except for the check to make sure the exact match is always first. There either is only 1 exact match at this point, or a list of partial matches.

  • If you're in interactive mode, it will ask the usual questions for the partial matches.
  • If you're using the .gitify file and there's no exact match, you're at the mercy of the returned sort order. The first match will be used.
  • Try installing 'Hits' for example. The package is no longer listed as available for the latest versions of MODX, so a list of partial hits hits (jaja) is returned and a different package is installed.
  • To prevent that from happening, you can use the full signature: 'hits-1.3.0-pl' will install the correct version. I also used this before with some other packages.

So it seems any previous version of a MODX package can be accessed this way. Unfortunately, that trick doesn't work for the ModMore provider, if it's not available as previous version (under the same name ;)).

I also don't think it should install a different package when an exact match could not be found (in the case of Hits), but return a warning or error instead. That's for another time though.


So long story short: this is ready for testing. I didn't remove the debugging lines in the output yet, so you can see what's going on inside the different arrays involved.

@muzzwood
Copy link
Contributor

Hey @hugopeek , I've tested this and it works beautifully. 🚀
I didn't get an older version of a package on either the modmore or modx providers unless I specified it.
I'll merge this and remove the logging statements.
Good stuff! :)

@muzzwood muzzwood merged commit 3b55af5 into modmore:master Oct 14, 2021
@hugopeek
Copy link
Contributor Author

Thanks @muzzwood! Glad this is sorted now :)

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

Successfully merging this pull request may close these issues.

4 participants