[Package Manager] Re-Download missing packages

[exside]

Was surprised that the package manager is not able to re-download packages in case the zip files / extracted folders are missing for an installed package (for whatever reason, in my particular case did I want to save space for backups and did not include the packages folder...). When trying to re-install a package with missing files it would proceed to the installation (of course without metadata displaying), then when hitting "Continue" show the modal and say could not download the package etc., but it never actually tried to re-download...

What does it do?

This PR changes that so if the zip file is not present, it would actually try to re-download it. There is an edge case where the package is not available anymore on the provider (for example I had an old version of the "console" extra that was not available anymore) and the provider would return an error code (e.g. 404 in that case), but the old code would actually "package" the response HTML or JSON code into a .zip file and then of course the installation would fail with "could not unpack package"...This PR also solves that issue by handling error codes from the provider.

Although not used very often, I did rewrite large parts of the _getByFsockopen() method as it needed some love.

Why is it needed?

See above.

[exside]

If anybody knows a better way to get a packages download link than (without making an unnecessary remote request)

$location = $this->get('metadata')[array_keys(array_filter($this->get('metadata'), function($item) { if ($item['name'] === 'location') { return $item; } }))[0]];

please let me know, happy to change that monster...

And of course it would be nice if the frontend would inform the user about whats happening before he hits "Continue/Install" and then gets the error, but not sure how to add that...

[OptimusCrime]

I do not know how the lookup is structured in $this->get('metadata'), but at the very least have the method on a seperate line:

$location = $this->get('metadata')[array_keys(array_filter($this->get('metadata'), function($item) { 
    if ($item['name'] === 'location') { 
        return $item; 
    }
}))[0]];

I would split this expression into a method instead and do each step seperate. Here the code is a bit too confusing to just read out. You also seem to use array_filter and then just use the first element in the resulting array. This could be split into a method that finds the correct $item or returns null, which, in turn, should execute the remote fetch instead.

[exside]

@OptimusCrime added your multiline suggestion, I'd like to avoid creating new methods for something "that trivial", as far as I can see if a package is installed it will always have the location in the metadata, so there should be no need for a remote request. The problem is, that the metadata is horribly structured to say the least, its an array with numeric keys, so the location was at index 28 in my case, but I don't want to rely on that index to be always the same, therefore the array_filter() which of course will always only return one item. Maybe we can take the multiliner a bit apart without creating a new method:

// loop trough metadata arrays until location is found
$locationarr = array_filter($this->get('metadata'), function($item) { 
    if ($item['name'] === 'location') { 
        return $item; 
    }
});
// determine the index at which the location metadata was found
$locationkey = array_keys($locationarr)[0];
// get the location metadata
$location = $this->get('metadata')[$locationkey];

will update like that for now.

[JoshuaLuckers]

To maintain readability I suggest using the camelCase naming convention for variable names.

[JoshuaLuckers]

Nice work! Just to be sure, did you sign the Contributor License Agreement?

[exside]

@JoshuaLuckers @OptimusCrime

Separated the metadata code into a separate, more generic method getMetadata(), like this a package's metadata can be accessed more easily in general, any other field as well, in case its needed.

    /**
     * Get metadata for a package in a more usable format
     *
     * @return array an array of metadata accessible by metadata name field
     */
    public function getMetadata() {
        $metadata = array_reduce($this->get('metadata'), function ($result, $item) {
            $key = $item['name'];
            unset($item['name']);
            
            $result[$key] = $item;
            
            return $result;
        }, array());
        
        return $metadata;
    }

That method will return an array in the form of:

Array
(
    [id] => Array
        (
            [attributes] => Array
                (
                )

            [text] => 4d556cf0b2b083396d000eeb
            [children] => Array
                (
                )

        ),
...
    [location] => Array
        (
            [attributes] => Array
                (
                )

            [text] => http://modx.com/extras/download/?id=4d556cf1b2b083396d000eec
            [children] => Array
                (
                )

        )
...
)

that reduces the code within transportPackage() to:

            /* make sure the package is downloaded, if not attempt re-download */
            if (strpos($sourceFile, '//') === false && !file_exists($targetDir . $sourceFile)) {
                /* get the package metadata */
                $metadata = $this->getMetadata();

                if (!empty($metadata) && $metadata['location']) {
                    /* assign remote download URL */
                    $source = $metadata['location']['text'];
                }
            }

Need some help with the sprintf() thingy, not sure how you would do that optimization.

@JoshuaLuckers signed the contributor agreement years ago =)

[OptimusCrime]

Not sure if we use sprintf that much in the core. I usually just see string concatenation like you have already done.

[exside]

Another update, did the direct return as suggested but also found some strange edge cases with an installation I have:

1. it failed because there was no location attribute on the first level, only in the nested array of the file attribute --> therefore I had to implement recursion into getMetadata(), so it also parses the nested arrays and falls back to $metadata['file']['children']['location']['text'] (yeah, my fingers hurt after typing that =D), the method got a bit uglier but also more versatile/complete. It's worth mentioning that I used use, but as the min. PHP requirements for MODX 2.6+ is PHP 5.3+, that should be fine right?

2. For some strange reason there are packages where a nicely formatted, one-dimensional attributes array is returned from $this->get('metadata) (newest MIGX and pThumb for example), had to deal with that as well...

There is an edge case I couldn't solve: Missing dependencies (like resizer for pThumb) are not properly re-downloaded, I had to delete the package via manager first, then re-install pThumb to make it work...don't know why^^, not familiar enough with the dependency feature of the package manager.

[Jako]

Just one thing. Could this PR target 2.x?

[JoshuaLuckers]

I don't think it has any influence on how the code works but just to be consistent it would be nice if the referer would use the same protocol (http or https) as the "host".

[exside]

@Jako just changed the base to 2.x, is that the proper way of targeting PRs? It seems it kind of messes up the PR a bit, there are now 10 changed files (probably because the PR was based on 2.6.5) instead of just one...do I need to resolve that by re-doing the PR based on the 2.x branch?

@JoshuaLuckers changed the way of returning metadata according to your suggestion and added the proper URL scheme to the referrer, guess that's the right way:

"Referer: " . MODX_URL_SCHEME . $host,

?

[JoshuaLuckers]

Looks fine to me! After you changes the target you have some conflicts that need to be solved. Best way to fix it is to create a new branch with your changes. Revert the current branch to the latest commit from 2.x and cherry pick your changes on to the current branch and force push them.

[Jako]

I am not sure, if it is the case. It should be possible to redownload every package, not only missing ones.

[opengeek]

Looks fine to me! After you changes the target you have some conflicts that need to be solved. Best way to fix it is to create a new branch with your changes. Revert the current branch to the latest commit from 2.x and cherry pick your changes on to the current branch and force push them.

@JoshuaLuckers — Don't fear the rebase! ;)

[exside]

@Jako A general Re-download feature would be another story, this is simply about fixing a broken "Re-install" functionality in case there is no zip-file present in the packages directory (or the unpacked folder is empty)...

regarding the branch mess, was that really necessary (to change the target branch to 2.x)? Switched back to the original 2.6.x for now...

[Ibochkarev]

@exside please accept @JoshuaLuckers suggestion and merge conflict.

[cla-bot]

Thank you for your contribution! Before your pull request can be reviewed, please sign the MODX Contributor License Agreement (CLA). This is to make sure MODX has your written permission to distribute projects containing your contributions under the appropriate license.

Please make sure the CLA has been signed for GitHub user(s): @exside

Once you've signed, please reply with @cla-bot check to verify and update the status of this pull request.

[Jako]

I have commited the changes of @JoshuaLuckers here.

[opengeek]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[JoshuaLuckers]

@Mark-H @opengeek any idea what's the best way to resolve the conflict(s) in this PR? Do we need to manually apply the changes to the refactored file?

[Mark-H]

Typically you'd need to do a rebase + force push. If that's not feasible or complex, manually applying changes In a new branch is sometimes easier.

[opengeek]

There are 31 commits in this PR. It needs to be reapplied manually on top of latest. It seems someone merged other work into this PR branch.