search

mitcho / shibboleth

WordPress Shibboleth plugin
23 stars 23 forks source link

Valid Changes from UW-Madison? #4

Closed jrchamp closed 7 years ago

jrchamp commented 10 years ago

Greetings! I was pleased to find your official repository here, just before I went ahead with a fork from a previous version by a guy from UW-Madison.

He has a couple interesting commits on his 'uw' branch: https://github.com/bshelton229/wp-shibboleth/commits/uw

Are these valid improvements that your codebase would benefit from? Specifically:

Thanks! Really looking forward to using this once my entityId is approved.

mitcho commented 10 years ago

Hello! Thanks for dropping by and bringing these up.

Would you mind creating branches for each of these proposed changes and issuing a pull request? That way we can have a clean thread discussing each issue and reach a resolution.

Finally, what's the entityId issue you mention? I didn't see them in the three changes you linked to.

mitcho commented 10 years ago

Actually, as I think about this more: @bshelton229: would you mind submitting these pull requests? I want to make sure the changes are attributed to you. Thanks!

bshelton229 commented 10 years ago

@mitcho Absolutely, I'll take a look this week and see if there is anything you haven't already patched, and open up PRs. It's great to have this on Github. I'll recommend UW-Madison start using this version, after testing. Some of our changes may have been to have UW-Madison defaults in the settings, but that's something I'm sure they'd gladly forego to be on the upstream version. Thanks so much for making this available!

jrchamp commented 10 years ago

@mitcho There was no entityId "issue" per se. I was requesting a new entityId from my IDP for the SP I was trying to set up so that I could use this plugin for WordPress authentication instead. They've now granted my request and I'm moving forward with testing. Thank you again!

Once @bshelton229 has created the relevant PRs, feel free to mark this as resolved. Thank you both for making my day!

mitcho commented 10 years ago

Thank you both! Looking forward to the PRs.

michaelryanmcneill commented 7 years ago

Hello, thank you for reporting this issue. I am the new maintainer of the plugin and all further work on the plugin will be done in a new GitHub repository. Have these PRs been resolved/submitted @jrchamp? If not, would you be willing to submit them for consideration to the new repository?

jrchamp commented 7 years ago

Two of them are still relevant so I've opened pull requests. Regarding the third (double URL encoding), my reading of the add_query_arg() function and relevant code indicates that it should be passed in already URL-encoded (which the code already does). WordPress core made the odd decision to not URL-encode in their version of build_query(), so we're stuck with that for now.

Closing.