? Success

User tests: Successful: Unsuccessful:

avatar piotr-cz
piotr-cz
19 May 2014

Fixes 2 issues ocuring during Extension updates:

  • the JHttpFactory::getHttpTransports method may return transports in random order (strange, I know), so the diver autoselection may pick JHttpTransportSocket instead of Curl.
  • support of follow_location to JHttpTransportSocket so it may handle 301-type responses.

Wikipedia says: A user agent should not automatically redirect a request more than five times, since such redirections usually indicate an infinite loop.

However JHttpTransportCurl doesn't set CURLOPT_MAXREDIRS, JHttpTransportStream uses max_redirects default 20 so let's rely on severs' sanity :D

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

Votes

# of Users Experiencing Issue
3/4
Average Importance Score
4.75

avatar piotr-cz piotr-cz - open - 19 May 2014
avatar piotr-cz piotr-cz - change - 19 May 2014
Title
Fix to JHttpTransportSocket
[#32468] Fix to JHttpTransportSocket
avatar Bakual
Bakual - comment - 19 May 2014

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

avatar piotr-cz
piotr-cz - comment - 19 May 2014

done

avatar xillibit
xillibit - comment - 7 Aug 2014

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

avatar piotr-cz
piotr-cz - comment - 8 Aug 2014

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

avatar xillibit
xillibit - comment - 8 Aug 2014
avatar nicksavov nicksavov - change - 21 Aug 2014
Labels Removed: ?
avatar piotr-cz
piotr-cz - comment - 28 Aug 2014

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

avatar xillibit
xillibit - comment - 28 Aug 2014

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

avatar roland-d
roland-d - comment - 28 Aug 2014

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/.

avatar pjwiseman
pjwiseman - comment - 28 Aug 2014

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/.

avatar pjwiseman pjwiseman - change - 28 Aug 2014
Status New Ready to Commit
avatar pjwiseman
pjwiseman - comment - 28 Aug 2014

@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/.

avatar Bakual
Bakual - comment - 28 Aug 2014

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

avatar mbabker
mbabker - comment - 28 Aug 2014

The redirect part is, yes.

avatar Bakual
Bakual - comment - 28 Aug 2014

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

avatar pjwiseman pjwiseman - change - 28 Aug 2014
Status Ready to Commit Pending
avatar pjwiseman
pjwiseman - comment - 28 Aug 2014

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/.

avatar pjwiseman
pjwiseman - comment - 28 Aug 2014

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/.

avatar Bakual
Bakual - comment - 29 Aug 2014

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. :smile:
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 :smile:

avatar piotr-cz
piotr-cz - comment - 6 Sep 2014

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.

avatar b2z
b2z - comment - 17 Sep 2014

@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/.

avatar pjwiseman
pjwiseman - comment - 18 Sep 2014

@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/.

avatar pjwiseman pjwiseman - change - 18 Sep 2014
Category Updating
avatar pjwiseman pjwiseman - change - 18 Sep 2014
Status Pending Ready to Commit
avatar roland-d
roland-d - comment - 4 Oct 2014

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.

151bb1e 6 Oct 2014 avatar infograf768 cs
0660c2f 6 Oct 2014 avatar infograf768 cs
avatar jissues-bot jissues-bot - change - 6 Oct 2014
Labels Added: ?
avatar piotr-cz
piotr-cz - comment - 6 Oct 2014

@roland-d
Rebased.

avatar Bakual
Bakual - comment - 6 Oct 2014

@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.

avatar piotr-cz
piotr-cz - comment - 6 Oct 2014

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

avatar Bakual
Bakual - comment - 6 Oct 2014

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 :smile:

avatar piotr-cz
piotr-cz - comment - 6 Oct 2014

How to fix it? :)

avatar pjwiseman
pjwiseman - comment - 6 Oct 2014

@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.

avatar Bakual
Bakual - comment - 7 Oct 2014

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 :smile:

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.

avatar pjwiseman
pjwiseman - comment - 7 Oct 2014

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.

avatar phproberto phproberto - close - 8 Oct 2014
avatar phproberto phproberto - close - 8 Oct 2014
avatar phproberto phproberto - change - 8 Oct 2014
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2014-10-08 08:23:47
avatar piotr-cz
piotr-cz - comment - 8 Oct 2014

thanks, @pjwiseman

avatar dgt41 dgt41 - reference | 469af74 - 13 Oct 14
avatar dgt41 dgt41 - reference | 56600dc - 13 Oct 14
avatar piotr-cz piotr-cz - head_ref_deleted - 20 Oct 2014
avatar rdeutz rdeutz - reference | e36daad - 24 Oct 14

Add a Comment

Login with GitHub to post a comment