[#32468] Fix to JHttpTransportSocket #3616

piotr-cz · 2014-05-19T12:36:10Z

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

Bakual · 2014-05-19T12:54:27Z

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.

piotr-cz · 2014-05-19T13:03:58Z

done

xillibit · 2014-08-07T13:06:39Z

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

piotr-cz · 2014-08-08T09:20:17Z

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

xillibit · 2014-08-08T09:28:46Z

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

I can put a comment into this one ^

piotr-cz · 2014-08-28T07:47:26Z

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

xillibit · 2014-08-28T09:31:55Z

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

roland-d · 2014-08-28T10:01:49Z

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

pjwiseman · 2014-08-28T13:20:35Z

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

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

pjwiseman · 2014-08-28T14:16:05Z

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

Bakual · 2014-08-28T18:33:44Z

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

mbabker · 2014-08-28T18:41:42Z

The redirect part is, yes.

Bakual · 2014-08-28T18:49:51Z

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

pjwiseman · 2014-08-28T23:03:21Z

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

pjwiseman · 2014-08-28T23:31:20Z

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

Bakual · 2014-08-29T07:58:56Z

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 😄

Fix downloads from HTTPS on servers without a global cacert.pem

Fix to Brian request : joomla#4414 <pre> Create a module with an end publishing date in the PAST. In the module list there is no visible change to its publishing status. It is still green. If you check an expired article you will see that the icon in the article list is orange not green to indicate it has expired. </pre>

Signed-off-by: Seth Warburton <seth@internet-inspired.com>

@RemcoJanssen

Move button before searchbox Add tooltip Fix remembering state. Simplify language string. Layout fixes. Updated icons, made them state aware. Updated language strings, made them state aware. Updated icons, alpha ordered language file, added grey background to the sidebar. Make the code abstract Added check if localStorage is available. Added code change to compressed core.js. Added toggle button to all views. Generated CSS files based on less generator. Fixed toggle buttons on user views. Update template-rtl.less. Fixed toggle button being activated on enter in filter field. Moved button to the sidebar file. Added transition changes. Cleanup banners Cleanup banners Further cleanup Further cleanup Simplify hiding the sidebar Keep the button visible Add missing empty line Updated the sidepanel. Many thanks to @RemcoJanssen Fixed Plugin view using the wrong div ID Updated CSS for RTL languages Change the icons used for closing and opening. Updated CSS for RTL.

This is probably a close tag from the default.php list view where the id is enclosed in a span element with a title set to the lft and rgt of the item. The starting tag was removed but the closing tag was still there. Removing the close tag results in a cleaner and valid html code.

… where used

…oomla#4183)

piotr-cz · 2014-10-06T08:50:52Z

@roland-d
Rebased.

Bakual · 2014-10-06T09:07:16Z

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

piotr-cz · 2014-10-06T09:22:25Z

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

Bakual · 2014-10-06T11:47:30Z

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 😄

piotr-cz · 2014-10-06T19:51:50Z

How to fix it? :)

pjwiseman · 2014-10-06T22:10:06Z

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

Bakual · 2014-10-07T06:19:56Z

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.

pjwiseman · 2014-10-07T11:25:34Z

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.

piotr-cz · 2014-10-08T20:11:42Z

thanks, @pjwiseman

joomla#3616. Fixes joomla#4474 Keep alphabetical order of detected transports across all environments Shortened description Keep alphabetical order of detected transports across all environments Added follow_location functionality to JHttpTransportSocket Shortened description

piotr-cz added 2 commits May 19, 2014 14:30

Added follow_location functionality to JHttpTransportSocket

f84caaa

Keep alphabetical order of detected transports across all environments

884b300

piotr-cz added Has JoomlaCode Tracker Item labels May 19, 2014

piotr-cz changed the title ~~Fix to JHttpTransportSocket~~ [#32468] Fix to JHttpTransportSocket May 19, 2014

piotr-cz mentioned this pull request May 19, 2014

Fix transports order and Transport\Socket joomla-framework/http#2

Merged

Shortened description

a8f37b4

piotr-cz mentioned this pull request May 19, 2014

Need to update XML to latest version guilleva/Xmap#9

Open

nicksavov removed the Has JoomlaCode Tracker Item label Aug 21, 2014

Michael Babker and others added 5 commits September 5, 2014 13:03

Merge pull request joomla#4232 from nikosdion/fix/https-download-fopen

7fafc1a

Fix downloads from HTTPS on servers without a global cacert.pem

Codestyle

9fae264

Added follow_location functionality to JHttpTransportSocket

ef9fd98

Keep alphabetical order of detected transports across all environments

d181d9d

Shortened description

5f48a7e

Michael Babker and others added 15 commits October 6, 2014 10:49

Add an Ant command for packaging com_joomlaupdate

5e35cc3

Fix in test case due to core changes of making variable protected

669b83e

Prepare 3.3.6 hotfix

26ddc8c

Reset for development

fe0c6a1

[imp] Removes self-linking article title. Fixes joomla#4402

9f91183

Signed-off-by: Seth Warburton <seth@internet-inspired.com>

Correcting wrong rtl in protostar

0cc15c1

Update tests to use the mock JApplicationCms object and mock JSession…

3e20f45

… where used

Add tear down function. Fixes joomla#4458

47242cf

Ensure vars are always set if the default menu item is retrieved (Ref j…

884df93

…oomla#4183)

Updating es-ES and fa-IR installation languages

e92a303

Added follow_location functionality to JHttpTransportSocket

9f59646

Shortened description

587c6b3

jissues-bot added the RTC This Pull Request is Ready To Commit label Oct 6, 2014

pjwiseman mentioned this pull request Oct 7, 2014

[#32468] Fix to JHttpTransportSocket (3616 rebase) #4474

Closed

phproberto closed this in 66f1656 Oct 8, 2014

piotr-cz deleted the patch_JHttp branch October 20, 2014 07:54

zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#32468] Fix to JHttpTransportSocket #3616

[#32468] Fix to JHttpTransportSocket #3616

piotr-cz commented May 19, 2014

Bakual commented May 19, 2014

piotr-cz commented May 19, 2014

xillibit commented Aug 7, 2014

piotr-cz commented Aug 8, 2014

xillibit commented Aug 8, 2014

piotr-cz commented Aug 28, 2014

xillibit commented Aug 28, 2014

roland-d commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

Bakual commented Aug 28, 2014

mbabker commented Aug 28, 2014

Bakual commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

Bakual commented Aug 29, 2014

piotr-cz commented Oct 6, 2014

Bakual commented Oct 6, 2014

piotr-cz commented Oct 6, 2014

Bakual commented Oct 6, 2014

piotr-cz commented Oct 6, 2014

pjwiseman commented Oct 6, 2014

Bakual commented Oct 7, 2014

pjwiseman commented Oct 7, 2014

piotr-cz commented Oct 8, 2014

[#32468] Fix to JHttpTransportSocket #3616

[#32468] Fix to JHttpTransportSocket #3616

Conversation

piotr-cz commented May 19, 2014

Bakual commented May 19, 2014

piotr-cz commented May 19, 2014

xillibit commented Aug 7, 2014

piotr-cz commented Aug 8, 2014

xillibit commented Aug 8, 2014

piotr-cz commented Aug 28, 2014

xillibit commented Aug 28, 2014

roland-d commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

Bakual commented Aug 28, 2014

mbabker commented Aug 28, 2014

Bakual commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

pjwiseman commented Aug 28, 2014

Bakual commented Aug 29, 2014

piotr-cz commented Oct 6, 2014

Bakual commented Oct 6, 2014

piotr-cz commented Oct 6, 2014

Bakual commented Oct 6, 2014

piotr-cz commented Oct 6, 2014

pjwiseman commented Oct 6, 2014

Bakual commented Oct 7, 2014

pjwiseman commented Oct 7, 2014

piotr-cz commented Oct 8, 2014