Remove follow redirects.

[dianaprajescu]

I have removed the option to automatically follow redirects for reasons specified in pull request #1432

[realityking]

How do the other transports behave in this regard?

[dianaprajescu]

I have used only curl so far.

[realityking]

Maybe someone else could confirm. I think it'd be good to be consistent between the transports.

[dianaprajescu]

Socket does not follow redirects automatically, I'm not sure about stream and curl wasn't following redirects either until @robschley made it to do so 2 moths ago: 3162faa.

[pasamio]

If you think it should be made an option then make it an option and default it to enabled.

[pasamio]

Also stream I believe by default will attempt to follow redirects:
http://php.net/manual/en/context.http.php

We should probably implement consistent behaviour across each and I'd suggest most of the time following redirects is a reasonable solution and should be the default.

[dianaprajescu]

It's not about what I think, I already told you that I need this.

You're talking about implementing consistent behavior across transports, but stream follows redirects and socket doesn't.

I thought it would be better not to follow redirects because the RFC says: 'The action required MAY be carried out by the user agent without interaction with the user if and only if the method used in the second request is GET or HEAD. A client SHOULD detect infinite redirection loops, since such loops generate network traffic for each redirection.' : http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

If you want I'll leave it to follow redirects and add an if block not to change it if specified through an option. In this case you'll have to consider setting curl CURLOPT_MAXREDIRS option too.

[elinw]

There are definitely situations in which redirects should not be automatically followed and Facebook is following the RFC in what it is expecting. The question then becomes how to handle the different behaviors in the 3xx range (just like in the 4xx range which is also problematic).

Also, just from searching a bit about this do we have a problem with CURLOPT_FOLLOWLOCATION if safe_mode or open_basedir are on? The platform hasn't raised the minimum to 5.4 so I would think that needs to be accommodated annoying though it is.
Also @dianaprajescu isn't your practical issue about images? I read a post that images add the header to the return and therefore the usual solution when safe-mode is on won't work.

@dianaprajescu I think people are talking themselves through this trying to come up with the best overall solution. Thanks for doing so much careful thinking and research about this.

[eddieajau]

For now, would it be ok to just change this to:

$options[CURLOPT_FOLLOWLOCATION] = (bool) $this->options->get('curl.followLocation', true);

If we can support it in the other transports it won't be hard to support it with a generic option. It would be good to document the options that are available, probably in the this class constructor Docblock.

Does that get you out of trouble?

[dianaprajescu]

Yes, this will solve my problem, I just thought it would be better not to follow redirects automatically. In this case it would be a good idea to set the CURLOPT_MAXREDIRS in order to specify the maximum number of redirects to follow, avoiding infinite loops.

[pasamio]

Instead of using a curl specific option, why not use a generic name like the stream options (follow_location or max_redirects) and then at the transport level map them onto their respective options. This will automatically work for stream as it passes in the HTTP options straight through and cURL will require some level of custom handling either.

Safe mode is gone in PHP 5.4 as well.

[dianaprajescu]

@elinw, yes, there is a problem with CURLOPT_FOLLOWLOCATION if safe_mode or open_basedir are on, but by using @eddieajau's suggestion you could just disable FOLLOWLOCATION.

[elinw]

Sounds good to me.

[elinw]

So I'm sure this won't make me popular but both this and the original change should be documented as b/c breaks and should be listed as such in the release notes when 12.2 is tagged. Rob's commit both, bug fixed a wrong behavior for most cases that wasn't being picked up by tests and it broke correct behavior in others that was not documented by tests. So with Andrew's approach and Sam's if we go that way we're maintaining b/c with what is there now which is ok but need to be documented.

We're not testing this at all either before or after but I don't think it's fair to say Diana has to write the tests for the whole series of events (though they would be nice to have) just to restore the part of the previously existing behavior that was right. Technically the code is "covered" but we don't know that it is actually working as in producing the correct results which is why no one thought about b/c when the original commit was made.
And I agree with Andrew about adding to the docblocks, @dianaprajescu if you would do that it would be great. And it would be good to add a note about the open_dir and safe_mode use cases maybe here or maybe in the manual.

[pasamio]

As I noted earlier instead of using a curl specific option, why not use a generic name like the stream options (follow_location or max_redirects) and then at the transport level map them onto their respective options. This will automatically work for stream as it passes in the HTTP options straight through and cURL will require some level of custom handling either. Otherwise you end up going set(curl.followLocation, true), set(follow_location, true), etc for each transport and that seems a tad silly because it reduces the effectiveness of swapping in and out different transports.

[elinw]

@pasamio and @eddieajau could you discuss and agree on which way to handle and then let us know?

[dianaprajescu]

This could be a small, fast fix just to allow my project to work properly 100% and then if someone wants could make the change @pasamio wants and I would also make the necessary changes in my project to work with it. Since there wasn't a previous discussion about curl options probably there was nobody to need them, so is this a priority right now?

I don't understand why are we arguing about this now while no one said anything when Rob first added that commit? Suddenly some of my project's methods weren't working because of this commit and now I'm just trying to fix it.

[pasamio]

So Rob made a change to default the cURL follow redirects behaviour to be inline with standard PHP stream handler where it follows redirects. This was to fix something Rob was working on where he expected cURL to be like PHP's stream handler it wasn't and now they're consistent.

This change broke your code. Unfortunately when sharing code and taking contributions these things happen. Your reaction is to create a pull request to revert Rob's pull request. Now since the change broke your code, your change is going to break his code if it were to be merged. Since Rob didn't know, or expect, this to break your code we can't expect him to have foreseen that making cURL behave like PHP streams would cause a problem. However you do know it will break his code because this change broke yours and knowingly breaking someone else's code is not very polite.

As an aside what this shows is that your code right now breaks on anyone's system that doesn't have cURL installed unless you're setting follow_location in $options already -- or you're outright forcing the transport to cURL which again means you will have problems on those systems. And if you're forcing the transport you're avoiding one of the primary reasons for the development of JHttp: transport neutrality.

So my suggestion is to implement consistent behaviour echoing Rouven's earlier suggestion. I also went and linked to the appropriate documentation of the options for stream with an indication that PHP stream's default behaviour is to follow redirects. In this sense Rob's change makes it consistent. Additionally as JHttpTransportStream uses $options for the context values we can also draw inspiration here to become consistent and support that in cURL. This goes back to an earlier request where you're trying to jam a $curlOptions variable into places.

What I want you to do is sit down and put the pieces together. I didn't make a comment and then 10 minutes later with a link to the PHP stream HTTP context options (hint 1: what does the stream transport do?) with the suggestion that it should be the default (hint 2: default it to follow) and that we should be consistent (hint 3: let's try 'follow_location' to be consistent) for fun. It was an attempt to guide your thinking and look at how the code works.

Nobody here is arguing, all we're after is flexibility and consistency. The reason there wasn't a discussion about adding cURL options previously was it wasn't thought to be necessary. Prior to Rob's change nobody did need them however clearly we do need them as you want a different behaviour. As noted by Rouven where possible we should be consistent in our behaviours across all transports. Rob's change made the cURL transport consistent with the PHP stream transport, so your change should continue in that spirit to be consistent with PHP stream's existing follow_location option. This means that you can set it to be off for what you need and it will work properly on both cURL and PHP stream's without other changes. Otherwise we're in the horrible land where you always set the option in case you're using one or the other and it breaks for you, again defeating the purpose of JHttp's transport neutrality.

And we all understand you're trying to fix your code being broken but as noted above knowingly and purposefully breaking a platform maintainers code does not usually endear you to them. We're aiming for a pathway which improves the flexibility of the code.

All I'm after is for you to rename it from curl.followLocation to a more portable follow_location so we don't have two different ways to set the same option - one for cURL and one for PHP streams for the same behaviour. You wouldn't change your light switch when you swapped a light bulb from incandescent to fluorescent to perhaps LED - why would we do the equivalent in code?

[dianaprajescu]

Thanks for telling me I'm not polite for nothing, the last change I made $options[CURLOPT_FOLLOWLOCATION] = (bool) $this->options->get('curl.followLocation', true); would not break anyone's code and it is also consistent with stream. This code will return the followLocation option value if there is one set and true otherwise, so it will follow redirects by default and it is consistent with stream. If there's something about this change I don't see please tell me, but I'm not breaking anyone's code knowingly and purposefully.

If all you're after is renaming the curl.followLocation option to follow_location then you could say so instead of writing lines and lines telling me that I'm breaking someone's code and this is not consistent with stream because it doesn't follow redirects.

[realityking]

@dianaprajescu
You're quite close. What people are requesting is that you use a transport agnostic name (like you just did) and handle in the different transports. At least JHttpTransportSteam has an option called follow_locationthat should map to the one you just introduced. (Also the defaults should be that we follow redirects)

Thanks for going trough with this!

[dianaprajescu]

I hope it's fine now.

[realityking]

Looks good to me, just waiting on the pull tester.

[pasamio]

@dianaprajescu I'm challenging you to think. Challenging you to think about a better solution and presenting what you're actually doing: you're knowingly breaking someone else's code and that's just plain rude and exposes rather bad manners. Clearly what I'm saying isn't getting through. If you don't want to think and be spoon fed like a baby then I can definitely treat you that way. In general I dislike treating people like idiots but I can definitely treat you accordingly if you wish to be treated that way. In my mind the entire point of GSoC is to expand your skills and one of the most important skills in programming is thinking. If you're not here to learn how to think then flipping burgers is where everyone tells you everything you need to do and don't expect you to think. I'd like to think you're better than that.

[HermanPeeren]

I'm really shocked by Sam's way of expressing himself. Maybe it's just the cultural difference, but to me the way it is written is just plain rude, exposes rather bad manners and above all: is unnecessary arrogant and offensive. I don't want to discuss that, it's just to let you know how I personally experience it.

Let's see it as a challenge to look at our own way of saying things. Let's stay factual and avoid moral expressions about persons.

I think postings like mine now, about the way of saying things rather than commenting on the code, should be kept to a minimum. I doubted about sending private emails instead. But choose this comment-box after some consideration; sorry if it would annoy you. Know it is posted with good intentions and not to start a rant. Please don't comment on this in this thread (private messages or a more appropriate spot always possible).

[pasamio]

@HermanPeeren Perhaps I'm disappointed that spending time to try and educate seems lost. Merely telling someone what to do doesn't help you learn anything beyond for a particular case you should do a particular task. Rote learning may be useful for basic knowledge and facts however it doesn't develop understanding and it doesn't develop the most important skill. Telling someone to do a particular task is an example of rote learning and in my mind not particularly useful.

I feel one of the hardest things about computing is that we don't spend enough time on the understanding phase of learning. There are plenty of books which teach you facts, plenty of web pages that have steps to resolve problems but in many cases there are much less examples of the delivery of understanding (though certainly available, not as common). The skill that understanding gets you is extra realisation. However the problem with understanding is that often there are steps left out. In mathematics, at least in my part of the world, we were always taught to list out the steps we took to get to the final answer. Computing sometimes has that but often can miss something simple: why did you pick this pathway over another?

Why is it bad to merely revert someones work when it broke yours? Because it will break theirs.
Why don't we want to propose changes that breaks others' code unnecessarily? Because generally this isn't a good way of endearing yourself to them.
Why are we now discussing adding options for something that didn't have options before? Because two different people feel that the same code path should behave differently. By adding an option we gain consistency and flexibility.

At each point there are decisions to be made around flexibility, trade offs and the like.

However if the response to that is "why did you write a long post explaining it all when all you want me to rename something" then one gets the feeling that the time spent to walk through the problem set and develop understanding is lost and we're back to the situation where we're not thinking: we're merely being told what to do. Perhaps in a sense all we're doing is reacting again which was the problem in the first place and while there are certainly times for rapid reactionary changes, I'd like to think that when the platform encounters a conflict we try to think about it a little bit instead of just reacting.

What I feel differentiates the discipline of software engineering (though I dislike the term 'engineering') from the other engineering disciplines is that while we may think that building software is a hard science that we can place rigid engineering principles around the reality is much more like an art where creativity and thinking is required to build great solutions.

There are plenty of people in the IT industry who don't spend time to think, don't spend time to understand and consequently don't produce the great work. I reiterate my challenge: think. It's only through thinking that we'll get better solutions to problems, not reactionary ones.

[HermanPeeren]

Dear Sam, I totally agree with the content of what you say/said. I have the same frustrating experience of many people not thinking themselves. I also prefer to look for fundamental solutions instead of temporary fixes. And for me the need for creativity is the main reason why I like the "art" of programming so much too.

My only point was the way it was said. I think that your message would have been understood better if it had been communicated in a different way.

• with better communication a message is delivered more efficiently. In the things you wrote were some "jamming stations" with an opposite effect; i.c. moral (good/bad) statements about a person. It was only in a few phrases, but one rotten apple can ruin the whole basket.
• in any group, but maybe even more in a community driven open source project, the social aspect and way we respectfully value each other is equally or more important for the quality of the "product" than the way we design our software. To win an ocean sailing race around the world the teamwork is one of the most important aspects; probably even more than the technical condition of the ship.

The best way to learn others to communicate better is to show good examples of it yourself. That is a constant learning process, training our "egos" to stay in the background.

Thank you for all the time you took to react. But as said before: if there is any need to continue this conversation, we'd better find a more appropriate place for it.

[pasamio]

@HermanPeeren I think the problem is that I needed to say it easily 6 different times. I made comment on an earlier pull request, I made two comments here, I made a further comment on a commit and then again I made it on another commit and finally the last two that you see here. Each time I increase my pointedness until the point gets across until such time as the point gets across. The fact that you comment means that it's past the point of being obvious beyond the barriers of culture and language. I suggest you look at the entire conversation if you haven't already. It isn't until the last two comments where I suggest that directly reverting someones code completely is a bad thing (the original version of this pull request proposed in 505ea4f directly deletes Rob's original change which will likely immediately break the behaviour he's expecting which itself is consistent with how PHP's HTTP streams implementation works) and suggesting that this might not be the right way to do things. Earlier I suggest it should be an option and Rouven suggests it should be consistent. I wonder, how many time do you need to send the same message to the person with increased intensity until they get it? Clearly writing a detailed explanation of the thought process didn't do it. Posing simple questions of why this should be distinct from the established norm didn't do it either. Nor did linking to existing documentation on how it should behave or posing the question why this should behave different to the existing PHP implementation which itself was never answered. This worries me that direct questions are posed but never answered.

If anything this is the right forum to have this conversation, it's a discussion on how a pull request should evolve, how it should be reviewed and how the submitter can improve. An understanding on how this pull request could have been done better from the get go for all six or so parties now involved. More importantly if discussions aren't held here how can anyone hope to learn?

So I ask the question, should I have just told the student what to do in the first place or should I have encouraged them to think and learn on their own? (even though this seems fruitless in hindsight)

[elinw]

So ... there were a lot of problems with this whole series of pull requests, responses to comments, non responses to comments, responses to responses. I would say overall, we could look at this whole history and see why Diana made the pull request she did (and how it was successful in getting people to pay attention to the impact of the earlier merge where her previous attempts to do that were not getting any attention) but also that in terms of the actual code it contained it should have been thought out better. It's frustrating when someone else breaks your code, but that doesn't make what they did necessarily something that should be reverted, just something that needs to be finished. And that's what ended up happening.
So working together with a lot of other people to make good code is sometimes hard, and sometimes things get heated. Working on the web the heated stays in the Google cache forever, but hopefully in a few months you'll look back at this and say wow that was really intense and dramatic and now we're working together so well.
I am commenting in part in case anyone reads this in the future and says "What was that all about?" will know that there have been (as Herman suggested) lots of private conversations about it, and that's probably the best way to close it out.

[HermanPeeren]

Reading all the bits an pieces back (pfff...) it's nice to see that in the end the solution is in line of Diana's original proposal a month ago: we should use a parameter for following those redirects and the default value should be by a standard.

These postings were for me also a warning to stay carefull with interpreting people's intentions and to reread a discussion when things get heated (although that took quite some time). My principle is that everybody contributing to the project has good intentions. Nobody is doing things just to knowingly and purposefully breaking someone's code (platform maintainer or not is not relevant).

I'm glad I learned something from it. I personally want to put more effort in improving myself than in "educating" others.