Skip to content

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

Open
stepmuel wants to merge 1 commit into
mitcho:masterfrom
stepmuel:master
Open

fix 'undefined index' notice when testing for checkboxes#23
stepmuel wants to merge 1 commit into
mitcho:masterfrom
stepmuel:master

Conversation

@stepmuel

Copy link
Copy Markdown

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

@jrchamp

jrchamp commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

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

ghost commented Aug 29, 2016

Copy link
Copy Markdown

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 #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

ghost commented Aug 29, 2016

Copy link
Copy Markdown

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

Copy link
Copy Markdown

@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.)

michaelryanmcneill/shibboleth@eb54176

@ghost

ghost commented Apr 18, 2017

Copy link
Copy Markdown

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

ghost commented Apr 18, 2017

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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

@michaelryanmcneill

Copy link
Copy Markdown

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

ghost commented Aug 24, 2017

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants