shibboleth icon indicating copy to clipboard operation
shibboleth copied to clipboard

fix 'undefined index' notice when testing for checkboxes

Open stepmuel opened this issue 8 years ago • 9 comments

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

stepmuel avatar Jul 11 '16 20:07 stepmuel

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.

jrchamp avatar Jul 11 '16 21:07 jrchamp

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 avatar Aug 29 '16 16:08 ghost

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

ghost avatar Aug 29 '16 19:08 ghost

@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

michaelryanmcneill avatar Apr 18 '17 16:04 michaelryanmcneill

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 avatar Apr 18 '17 17:04 ghost

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.

ghost avatar Apr 18 '17 19:04 ghost

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

michaelryanmcneill avatar Apr 19 '17 03:04 michaelryanmcneill

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.

michaelryanmcneill avatar Aug 24 '17 00:08 michaelryanmcneill

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.

ghost avatar Aug 24 '17 17:08 ghost