Bugfix: issues#131 in Gitify

[tyllo]

modmore/Gitify#131

[Mark-H]

Hi @tyllo! Could you use the template from https://github.com/modxcms/revolution/blob/2.x/CONTRIBUTING.md#template-1 in the pull request, and explain a bit how this fix works?

[tyllo]

Google translate:
This patch fixes an oversight. When row $url =$this->downloadUrl sought url, then the method protected functiondownloadUrl "port" setting $ this->xpdo, because when you assign $rest =$this->xpdo->getService('rest','rest.modRestClient'); - Assigned to the link, and not copied to the variable $rest.
When downloading a single package, this error does not climbs, and for batch downloading - she climbs.

Russian:
Этот патч исправляет оплошность. Когда в строке $url = $this->downloadUrl ищется url, то в методе protected function downloadUrl "портится" настройка $this->xpdo, потому что при присваивании $rest = $this->xpdo->getService('rest','rest.modRestClient'); - присваивается по ссылке, а не копируется в переменную $rest.
При единичном скачивании пакета эта ошибка не вылазит, а при пакетном скачивании - она вылазит.

[graphiclunarkid]

I've tested this pull request on my Vagrant image and it fixes issue modmore/Gitify#131 for me 😸

[OptimusCrime]

Any reason this is not merged yet?

[graphiclunarkid]

Just checking in to see if there's any more testing I can do, or help I can give, to get this fix merged?

[hansek]

I got into same troubles with latest Gitify and MODX 2.4.2-pl, after this fix install:package --all working fine again.

[hansek]

Latest Gitify works fine with MODX 2.3.6-pl, even without this fix.

[pixelchutes]

Hi @tyllo! Any update on this? Just want to understand the how/why behind the solution.

(See @Mark-H's request above about updating to use the PR contribution template)

[Mark-H]

It does fix the issue we've been seeing with Gitify's batch package install, I've since tested this on several sites.

I think @tyllo might not speak a lot of English and after a bit more digging, I understand what this is doing as well, so if you'd allow me (as the developer responsible for Gitify :P), here's the template and description of what this does:

What does it do

Ensures the responseType is set correctly on the rest service. In the downloadUrl method of the provider object, the responseType is being set to text, as that request to the actual transport provider does not return XML (like all other requests), but a simple string as download url. To anticipate that, the responseType on the rest service is being set to text before the request is triggered (line 326/327).

The bug starts happening when multiple packages are installed in a single request (through Gitify package:install --all), as the responseType is not set back to its default value of xml on the service. (Only a single rest service instances is used throughout the request thanks to the getService calls). This causes issues on subsequent calls to the provider to list or download a certain package.

Why is it needed

This pull request makes sure the responseType is set back to its original value after the call to get the download url with responseType=text. That way batch downloads/installs with tools like Gitify work again as they did in 2.3.x.

Related issue(s)/PR(s)

modmore/Gitify#131

On a final note, I believe this should probably target 2.4.3 instead of 2.5 as it's a bug fix for behaviour that was inadvertently broken in 2.4.0 @rtripault.

[pixelchutes]

Thanks Mark. I'd agree, definitely 2.4.x. Is there a signed release?

[hansek]

Thanks guys, hope this will be released in next 2.4.x release ;)

[rtripault]

Right, this is indeed a fix for commit cb48cc3.
Sorry for not checking

[Mark-H]

@pixelchutes If by signed release you mean the CLA, then I'm not sure..

@tyllo Have you ever signed the MODX CLA? If not, can you please do so at modx.com/cla and post here when you did that?

[tyllo]

I am not registered to MODX CLA, I'm bad at writing English. If is still actual, you can take my commit themselves to recycle, I will not be offended.

[Mark-H]

Hi @tyllo, this pull request is still actual, thanks :)

If you could just go to http://modx.com/cla and fill in the form, that will allow us to merge it and include it in the next release. The CLA gives the MODX project the legal right to the code you provide, so it can be licensed as GPL.

[tyllo]

done

[pixelchutes]

Merged via d1f0555

[hansek]

👏

[graphiclunarkid]

❤️