Can you cut down the comment to one line? We don't need a full paragraph there.
Something like "Follow http redirects" would be enough.

done

Thanks for this fix, it fix for my extension. In some configuration i have only sockets extension enabled and not curl, in this case when i want update my extensions i have an error 902 displayed : http://screencast.com/t/JsoVuqqNcsi7

I wonder if we should not create new Joomla Issue tracker item and Add tests there

Related Issue tracker:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32468

I can put a comment into this one ^

@xillibit Since the JoomlaCode is not used anymore, I guess we have to lob on google groups

I know that now we must use Jissue, do-you will open a discussion on google groups ?

Why a discussion on Google Groups? You can discuss this tracker, right here.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

Waiting a few months is fairly standard.

To improve the speed at which the issue can be progressed, it would help to have test instructions.

"Every Pending issue should have instructions that tell the tester how to reproduce the problem and make sure the patch fixes the problem." http://docs.joomla.org/Bug_Tracking_Process

From a code review perspective, I'm asking myself the following questions, which for me to mark a successful test would need to be satisfied. (NB: Others with more experience might waive them as unnecessary questions).

1) Which is the best order for the transports? Looks like there previously wasn't any consideration of a best order. Are you proposing that alphabetical is the best order? (curl, socket, stream).

2) Is the sort() sufficient? ie. is it a binary sort or a language sort? Will it change in various locales? I'm guessing that's unlikely though not impossible. Satisfied - sort() is binary when not passed any sort flags.

3) Rather than sorting, can the order be pre-defined? That's harder as the order is currently only order returned by the DirectoryIterator, which looks like it's not guaranteed to be alphabetical. Can you confirm that the root of the problem is that the DirectoryIterator is returning the files (curl, socket, stream) in a non-alphabetical order? Or is there something else happening? (i.e. I'd like to be certain that the order isn't somehow user configurable). Satisfied - A little googling shows DirectoryIterator is essentially random.

Based on a cursory code review, I'm included to give it a pass as it looks unlikely to cause any regression.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@test Tested successfully.
The sort is covered by unit tests in the framework. I'm going to assume that works.
Tested the socket method by removing curl and streams from the http/transport directory.
Tested redirect by adding a custom redirect on my local server to the joomla update repository. Fails before applying the patch, works with patch applied.

As this has already been committed into the framework, I don't think it needs another test. Moving to RTC.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

Is this still needed after we merged #4105? I think it achieves the same thing.

The redirect part is, yes.

Ah true. Then the PR should be rebased to current staging and the sort removed (which probably resolves the conflicts).

Changing back to pending until the conflict is resolved. Using "sort" aligns with jooomla-framework. Using the "curl" preference doesn't. A decision will need to be made.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

As the stream transport doesn't currently support http redirects, I think the preferred order should be curl -> socket -> stream. i.e. Use "sort", which also benefits as it matches the joomla-framework.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

Personally I wouldn't rely on sort just because you don't know if in future there may be a better transport which starts with eg z.
The current way isn't ideal neither. Best would be if we could add priorities somehow within the transport class itself.

But for now I'm fine either way. Just remove the conflicts

Rebased to staging.

Priorities would be best, but at this moment transport types are being registered by:
1) being available in folder
2) being supported in system.
3) CURL is prioritized (#34065)

I decided to use sort, because this keeps transport order consistent across all systems.
The fact that the alphabetical order is most probably the wanted one is a positive side effect.

@pjwiseman or @piotr-cz can you provide more clear instructions how to test? Thanks ;)

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@test
Retested successfully.

• PR now merges successfully.
• The sort is covered by unit tests in the framework. I'm going to assume that works.
• Tested the socket method by removing curl.php and streams.php from the libraries/joomla/http/transport directory.
• Tested redirect by adding a redirect file on my local server to the standard joomla update repository, and updating the joomla update options to point to the redirect file. header("Location: http://update.joomla.org/core/sts/list_sts.xml");

Redirect fails before applying the patch, works with patch applied.

As this has already been committed into the framework, I don't think it needs another test. Moving to RTC.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

Hello,

Been trying to test this but the patch is out-of-sync with the current staging branch. Can you please update the PR so I can do the test?

Thanks.

@roland-d
Rebased.

@piotr-cz I think your rebase went wrong. It looks like you applied all commits from staging into your branch after your own commits.
A proper rebase would add those commits before your own commits, changing the history of your branch.
Alternatively you can create a merge commit to merge the new commits from staging into your branch.

Rebasing is preferred as it maintains a clean PR. However merge-commits are simpler as you just can merge the upstream staging.

@Bakual you are right, can I undo it now or create new PR?

You have two options:

• You can fix it and then force-push your branch so that it overwrites the one in your fork.
• You close this one and create a new PR.

Both ways work fine

How to fix it? :)

@bakual I had moved this to RTC on 18 Sep, and now it needs re-basing? Any idea what the blocker was stoping it from being committed?

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3616.

How to fix it? :)

@piotr-cz You can try to checkout an earlier version of your branch and work from there again. Or you just create a new branch based off staging and apply your changes to this clean branch. You can do that by cherrypicking the commits or by manually do the same changes again (depending how much it was you changed).
After you have your clean local branch with only your own changes, you can force-push it to your fork overwriting the existing branch from this PR.
Welcome to advanced Git

I had moved this to RTC on 18 Sep, and now it needs re-basing? Any idea what the blocker was stoping it from being committed?

@pjwiseman Apparently the PR was in conflict with another change made recently.

I've created a new PR#4474 rebased to master.

@piotr-cz I did the following... Feel free to update your PR (which will be cleaner than using mine.)
git checkout master
git pull
git fetch pull/3616/head:3616
git checkout 3616
git checkout -b 3616-rebase
git rebase master (and fixed the resulting conflicts)
git push pjwiseman 3616-rebase


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3616.

thanks, @pjwiseman