mitcho / shibboleth

WordPress Shibboleth plugin
24 stars 23 forks source link

fix 'undefined index' notice when testing for checkboxes #23

Open stepmuel opened 7 years ago

stepmuel commented 7 years ago

Small fix to get rid of some annoying PHP warnings when submitting the settings panel form… 

jrchamp commented 7 years ago

That makes sense for checkboxes. Worth nothing that the equivalent to (boolean) would be !empty() instead of isset(). Example: Radio buttons with values of 1 and 0.

ghost commented 7 years ago

Hello, I have tested this correction and it looks like it is still there is still a error (but likely minor).

I see the error "Undefined index: managed in..." when the fields are unamanaged.

Maybe a conflict with https://github.com/mitcho/shibboleth/pull/21 that I also have merged in my fork that has a small changed related to managed fields.

I will give it a second look and report if this can be useful.

ghost commented 7 years ago

The problem is there :

checked($shib_headers['first_name']['managed'], 'on')

When the value is unchecked, 'managed' key (index) doesnt exist.

This is what I have done to correct it: https://github.com/devanonyme/shibboleth/commit/da436adb8c5a91210aa14c4c9a5ac9e96d7bcb2c

michaelryanmcneill commented 7 years ago

@devanonyme I'm unable to reproduce the undefined index error that you've received. If you can pull down a copy of my repo with these PRs applied and let me know if you still see the error, I'd appreciate it. (I know this has been a while, so you may no longer be interested in this fix.)

https://github.com/michaelryanmcneill/shibboleth/commit/eb54176e017542d352e6342ec2f69786f9a91dd3

ghost commented 7 years ago

I will take a note to test it but it will be on a (very) slow track as I'm currently assigned to another project. Thanks!

ghost commented 7 years ago

I did the test with a few combination (keyname + unmanaged, keyname + managed, no key + unmanaged, no key + managed) and I was not able to reproduce the problem with your version. I noticed that the difference with your version aside the error handler with the other fix I had merged (https://github.com/devanonyme/shibboleth/commit/c763a49f44f81b2f814c57f0f51203ba384c0d76) was the use of !empty instead of isset, I guess the table of truth is not exactly the same... I probably didn't noticed that at the time and tried to fix it at the symptom level instead of the root.

michaelryanmcneill commented 7 years ago

Excellent news, thanks @devanonyme. Just wanted to make sure that I wasn't missing a test case in my changes.

michaelryanmcneill commented 6 years ago

Hello, thank you for submitting this patch. I released version 1.8 today to resolve this and other issues and included a shoutout for your patch. I am the new maintainer of the plugin and all further work on the plugin will be done in a new GitHub repository. If you have any further issues, please don't hesitate to report them in the new repository.

ghost commented 6 years ago

Thanks!

I think I had a problem with (federated) logout but I think there where no generic solution to this one. Other than that the plugin is doing the job that we need just fine.

Thanks also for taking the responsability of maintenance.