PSR-2 fix for ADFS module #481

tvdijen · 2016-09-28T20:36:23Z

I intentionally skipped whitespace-fixes, since there's a separate issue 458 for that.

coveralls · 2016-09-28T20:44:22Z

Coverage remained the same at 19.696% when pulling 281c936 on tvdijen:patch-5 into 644b05a on simplesamlphp:master.

jaimeperez · 2016-09-29T07:41:23Z

Thanks a lot @tvdijen!

#458 is basically a reminder to keep track of what we need to do, but that doesn't mean we need to make all white spaces in the project compliant with PSR-2 at the same time, in the same commit. Actually, I think it's better if we do that slowly, file by file. So I would say that's something you can address here too, and then we won't have files that are PSR-2 compliant except for the whitespace.

You could probably squash the existing commits, and then add another one changing tabs for spaces. Does that sound reasonable?

tvdijen · 2016-09-29T09:15:28Z

Sure, I can fix the whitespace too.. I just don't have any clue on how to squash these commits. Is that even possible with this online GitHub-interface?

jaimeperez · 2016-09-29T09:29:12Z

Great! I'm not sure you can squash them through the web interface yourself, as I think that's only for those with rights to merge the PR, but you can always do that in the command line.

tvdijen · 2016-09-29T09:34:52Z

That's, I'll check that out at a later time and then add a commit to fix whitespace too!

tvdijen · 2016-09-29T20:49:06Z

I think I screwed up..

coveralls · 2016-09-29T20:53:59Z

Coverage increased (+0.1%) to 19.802% when pulling 74975c4 on tvdijen:patch-5 into 644b05a on simplesamlphp:master.

jaimeperez

Thanks for your work @tvdijen!

I left some comments on the code. Don't worry about the squashing, that's something we can do easily (I think!)

jaimeperez · 2016-10-11T07:55:48Z

modules/adfs/lib/IdP/ADFS.php

-		$sessionLostURL = NULL; // TODO?
-		$forceAuthn = FALSE;
-		$isPassive = FALSE;
+		//$sessionLostURL = NULL; // TODO?


Is this a typo, or the variable is indeed unused?

If the latter, I would just get rid of it and if we need to mark something to do, leave just a comment, no code...

jaimeperez · 2016-10-11T07:56:44Z

modules/adfs/lib/IdP/ADFS.php

-		exit;
+	public static function adfsPostResponse($url, $wresult, $wctx)
+    {
+		print


Should we change this with a call to echo or a heredoc?

jaimeperez · 2016-10-11T07:58:12Z

modules/adfs/lib/IdP/ADFS.php

@@ -43,54 +45,62 @@ public static function receiveAuthnRequest(SimpleSAML_IdP $idp) {
 		$idp->handleAuthenticationRequest($state);		
 	}

-	public static function ADFS_GenerateResponse($issuer, $target, $nameid, $attributes) {
+	public static function adfsGenerateResponse($issuer, $target, $nameid, $attributes)


If the function is only used in this module, then it's ok to rename it. But in that case, I would take the chance to drop the adfs in front, since it's kind of redundant...

On the other hand, such change could probably be part of another PR, if this is only focused on PSR-2 compliance.

jaimeperez · 2016-10-11T08:00:32Z

modules/adfs/lib/SAML2/XML/fed/Endpoint.php

 	/**
 	 * Add this endpoint to an XML element.
 	 *
 	 * @param DOMElement $parent  The element we should append this endpoint to.
 	 * @param string $name  The name of the element we should create.
 	 */
-	public static function appendXML(DOMElement $parent, $name, $address) {
+	public static function appendXML(DOMElement $parent, $name, $address)
+    {


Looks like there's an extra space before the {?

jaimeperez · 2016-10-11T08:01:25Z

modules/adfs/lib/SAML2/XML/fed/Endpoint.php

 		assert('is_string($name)');
 		assert('is_string($address)');

 		$e = $parent->ownerDocument->createElement($name);
-                $parent->appendChild($e);
+        $parent->appendChild($e);


Same here and in the next change, looks like something is odd with indentation. Maybe the rest of the code is indented wrong?

jaimeperez · 2016-10-11T08:09:31Z

modules/adfs/www/idp/prp.php

@@ -23,6 +23,6 @@
 	// logout response from ADFS SP
 	$assocId = $_GET['assocId']; // Association ID of the SP that sent the logout response
 	$relayState = $_GET['relayState']; // Data that was sent in the logout request to the SP. Can be null
-	$logoutError = NULL; /* NULL on success, or an instance of a SimpleSAML_Error_Exception on failure. */
+	$logoutError = null; /* NULL on success, or an instance of a SimpleSAML_Error_Exception on failure. */


I would update the comment too (s/NULL/null/). In general, I also prefer single-line comment markers (//) for single-line comments, and multi-line comment markers (/* */) for multi-line comments, but that's more personal preference and you don't have to do that if you don't want to 😄

coveralls · 2016-10-17T12:13:35Z

Coverage increased (+0.5%) to 20.213% when pulling eed9afd on tvdijen:patch-5 into 644b05a on simplesamlphp:master.

tvdijen · 2016-10-17T12:18:35Z

Anything else @jaimeperez ?

coveralls · 2016-10-17T12:19:30Z

Coverage increased (+0.5%) to 20.213% when pulling a7e206e on tvdijen:patch-5 into 644b05a on simplesamlphp:master.

coveralls · 2016-10-17T12:23:46Z

Coverage increased (+0.5%) to 20.213% when pulling b5fb3c0 on tvdijen:patch-5 into 644b05a on simplesamlphp:master.

coveralls · 2016-10-17T12:28:33Z

Coverage increased (+0.5%) to 20.213% when pulling b5fb3c0 on tvdijen:patch-5 into 644b05a on simplesamlphp:master.

jaimeperez · 2016-10-24T08:41:03Z

Thanks a lot Tim! Looks good to me 👍

There's only one thing I've just realized. Since some of the methods are changing their names and they are declared public, refactoring them could break other code. Have you checked that those functions are not used elsewhere? The simplest solution would be to add again the old methods as wrappers of the renamed ones, and mark them as deprecated in the phpdoc block, but it might not be worth the trouble if they are so specific that nobody should use them.

tvdijen · 2016-10-24T12:04:31Z

I did a grep and the functions are only being used within the ADFS.php file.
Could probably make them private too.. I happen to have an ADFS machine available, so I can run a quick test if nothing breaks.. I'll get back to you on this.

jaimeperez · 2016-10-24T12:05:23Z

Sounds great, thanks so much Tim!

thijskh · 2016-11-14T10:45:56Z

Any update @tvdijen?

tvdijen · 2016-11-14T12:00:53Z

No, unfortunately I haven't found the time to run the test..
I'll see if I can fit it in somewhere this week, since I will be abroad for the next two weeks..

thijskh · 2016-11-14T12:02:27Z

Would be great, since I don't have a test environment for this stuff.

tvdijen · 2016-11-16T17:46:10Z

I wonder if this module ever worked at all.. I can't even get a working setup without this PR :(
I'm running into a 'Missing AuthState parameter' after logging in ...

pgh70 · 2016-12-13T11:10:40Z

I am using the ADFS module from 1.14.3, first without modifications, now with modifications from PR #469, so it did work!

tvdijen · 2016-12-13T16:19:10Z

That's great news @pgh70! Since you have a working environment, would you be willing to perform another test after I convert some functions from public to private?

thijskh · 2017-01-30T10:14:35Z

@pgh70 Can you test Tim's latest changes? Thanks!

tvdijen · 2017-08-18T21:21:56Z

This works like a charm!

jaimeperez added enhancement low labels Sep 29, 2016

jaimeperez requested changes Oct 11, 2016

View reviewed changes

jaimeperez approved these changes Oct 24, 2016

View reviewed changes

tvdijen force-pushed the patch-5 branch 2 times, most recently from e8859bb to b97f61d Compare July 29, 2017 13:27

tvdijen mentioned this pull request Aug 18, 2017

Flexible namespace #678

Closed

PSR-2 / Whitespace

6c4b3f2

Revert: set enctype

be7e8a3

tvdijen force-pushed the patch-5 branch 4 times, most recently from d41a0fe to 815a58a Compare August 18, 2017 21:21

Refactoring

da4ef8d

tvdijen force-pushed the patch-5 branch from 815a58a to da4ef8d Compare August 18, 2017 21:23

thijskh approved these changes Aug 19, 2017

View reviewed changes

thijskh merged commit 133fd05 into simplesamlphp:master Aug 19, 2017

thijskh mentioned this pull request Aug 19, 2017

ADFS-module: Set enctype on post-form #675

Merged

tvdijen deleted the patch-5 branch August 19, 2017 09:10

github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSR-2 fix for ADFS module #481

PSR-2 fix for ADFS module #481

tvdijen commented Sep 28, 2016 •

edited

Loading

coveralls commented Sep 28, 2016

jaimeperez commented Sep 29, 2016

tvdijen commented Sep 29, 2016

jaimeperez commented Sep 29, 2016

tvdijen commented Sep 29, 2016

tvdijen commented Sep 29, 2016

coveralls commented Sep 29, 2016

jaimeperez left a comment

jaimeperez Oct 11, 2016

jaimeperez Oct 11, 2016

jaimeperez Oct 11, 2016 •

edited

Loading

jaimeperez Oct 11, 2016

jaimeperez Oct 11, 2016

jaimeperez Oct 11, 2016

coveralls commented Oct 17, 2016

tvdijen commented Oct 17, 2016

coveralls commented Oct 17, 2016

coveralls commented Oct 17, 2016

coveralls commented Oct 17, 2016

jaimeperez commented Oct 24, 2016

tvdijen commented Oct 24, 2016

jaimeperez commented Oct 24, 2016

thijskh commented Nov 14, 2016

tvdijen commented Nov 14, 2016

thijskh commented Nov 14, 2016

tvdijen commented Nov 16, 2016 •

edited

Loading

pgh70 commented Dec 13, 2016

tvdijen commented Dec 13, 2016

thijskh commented Jan 30, 2017

tvdijen commented Aug 18, 2017

PSR-2 fix for ADFS module #481

PSR-2 fix for ADFS module #481

Conversation

tvdijen commented Sep 28, 2016 • edited Loading

coveralls commented Sep 28, 2016

jaimeperez commented Sep 29, 2016

tvdijen commented Sep 29, 2016

jaimeperez commented Sep 29, 2016

tvdijen commented Sep 29, 2016

tvdijen commented Sep 29, 2016

coveralls commented Sep 29, 2016

jaimeperez left a comment

Choose a reason for hiding this comment

jaimeperez Oct 11, 2016

Choose a reason for hiding this comment

jaimeperez Oct 11, 2016

Choose a reason for hiding this comment

jaimeperez Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

jaimeperez Oct 11, 2016

Choose a reason for hiding this comment

jaimeperez Oct 11, 2016

Choose a reason for hiding this comment

jaimeperez Oct 11, 2016

Choose a reason for hiding this comment

coveralls commented Oct 17, 2016

tvdijen commented Oct 17, 2016

coveralls commented Oct 17, 2016

coveralls commented Oct 17, 2016

coveralls commented Oct 17, 2016

jaimeperez commented Oct 24, 2016

tvdijen commented Oct 24, 2016

jaimeperez commented Oct 24, 2016

thijskh commented Nov 14, 2016

tvdijen commented Nov 14, 2016

thijskh commented Nov 14, 2016

tvdijen commented Nov 16, 2016 • edited Loading

pgh70 commented Dec 13, 2016

tvdijen commented Dec 13, 2016

thijskh commented Jan 30, 2017

tvdijen commented Aug 18, 2017

tvdijen commented Sep 28, 2016 •

edited

Loading

jaimeperez Oct 11, 2016 •

edited

Loading

tvdijen commented Nov 16, 2016 •

edited

Loading