Closed GoogleCodeExporter closed 9 years ago
Sorry, about the crappy patch file. This is one is better.
Original comment by email.sh...@gmail.com
on 1 Nov 2013 at 3:40
patch file
Original comment by email.sh...@gmail.com
on 1 Nov 2013 at 3:40
Attachments:
Hi,
some comments:
- force_login is a property of Twitter, not OAuth in general, so the property
does not belong in the Consumer class.
- You appear to have some mixup with whitespace/tab in your editor. This leads
to a lot of whitespace changes, which must be avoided. (We don't want to just
change a file from one indentation style to another, nor do we want multiple
indentation styles in a file.)
- You are calling the deprecated version of addURLparameter.
- Find out why your autoloader isn't working, rather than including a
workaround.
- The twitter attributes you add probably don't belong as attributes. Possibly
as entries in the state array, but not as attributes to be distributed to SPs.
They should also be submitted as a separate patch. (In general, unrelated
changes should be separate patches.)
- If you need the authsource to be available in processing filters you should
add it to the state array when the IdP is launching the processing filters. It
should also be a separate patch.
- Avoid changes in the patch that don't actually change anything. Like the
quote-style change for the authentication URL.
Original comment by olavmrk@gmail.com
on 1 Nov 2013 at 8:30
[deleted comment]
Thanks Olav, as always value your quick feedback! gives me an opportunity to
improve. My comments below
- force_login is a property of Twitter, not OAuth in general, so the property
does not belong in the Consumer class.
* I agree, I had a feeling that was the case hence I couldn't find any
reference to force_login in the OAuth RFC no doubt twitter are the pioneers of
OAuth. I have removed the attribute from Consumer class, instead just passing
it as a parameter ( I hope this approach is acceptable )
public function getAuthorizeRequest($url, $requestToken, $redirect = TRUE,
$callback = NULL, $force_login = FALSE)
- You appear to have some mixup with whitespace/tab in your editor. This leads
to a lot of whitespace changes, which must be avoided. (We don't want to just
change a file from one indentation style to another, nor do we want multiple
indentation styles in a file.)
* This stupid IDE! I need to stick to something simpler. Do you prefer diff or
patch files? The diff I sent earlier was far better. I shall avoid this
nonsense in future, apologies.
- You are calling the deprecated version of addURLparameter.
* Well picked, I should have seen it! I have changed it to use the one with an
array parameter.
- Find out why your autoloader isn't working, rather than including a
workaround.
* Any hints? I am not very familiar with how the autoloading in SSP is hooked
up. Happy to start hacking and I also need to read this
http://simplesamlphp.org/docs/stable/simplesamlphp-modules
- The twitter attributes you add probably don't belong as attributes. Possibly
as entries in the state array, but not as attributes to be distributed to SPs.
They should also be submitted as a separate patch. (In general, unrelated
changes should be separate patches.)
* Agree, I will create a separate patch.
- If you need the authsource to be available in processing filters you should
add it to the state array when the IdP is launching the processing filters. It
should also be a separate patch.
* Agree, completely out of the scope of this patch
- Avoid changes in the patch that don't actually change anything. Like the
quote-style change for the authentication URL.
* I see, i will fix that Line 127
I will soon provide an up to date patch taking your comments in to
consideration. Thanks again.
Regards,
Shoaib
Original comment by email.sh...@gmail.com
on 5 Nov 2013 at 12:58
Added force_login in r3295 (preparation patches in r3293 and r3294).
Original comment by comel...@gmail.com
on 18 Nov 2013 at 5:25
Thank you for this :) A lot simpler solution to just make it part of the
$callback.
Original comment by email.sh...@gmail.com
on 19 Nov 2013 at 1:29
Just in case anyone is watching or looking at this for autoloader issue, turns
out there was a file default-disabled in my oauth directory.
I would have thought oauth module should be enabled by default. All I needed to
do was rename the file to default-enable.
Original comment by email.sh...@gmail.com
on 31 Dec 2013 at 12:15
Original issue reported on code.google.com by
email.sh...@gmail.com
on 1 Nov 2013 at 2:57