options: Remove uses of extract()

[jrchamp]

I'm very biased against extract(), largely because of the smattering of the warning messages about localizing arbitrary variables. This isn't as big of a problem because we control the values the function returns, but I don't want to encourage the use of the function anywhere as it's a little bit of magic combined with a little bit of danger.

[michaelryanmcneill]

@jrchamp: this change has created a good amount of php notices on options-admin.php.

Notice: Undefined offset: 0 in /opt/app-root/src/wp-content/plugins/shibboleth/options-admin.php on line 194

[jrchamp]

Okay, there's two ways to fix it. The easy one is change https://github.com/michaelryanmcneill/shibboleth/blob/2.1-alpha/shibboleth.php#L61

return array( 'value' => $value, 'constant' => $constant );

becomes

return array( $value, $constant );

The other way would require changing the other code to not use list() and use the return values as the array it is.

Side note - If we required at least PHP 7.1, we could use list keys: https://wiki.php.net/rfc/list_keys

list( 'value' => $value, 'constant' => $from_constant ) = shibboleth_getoption();

[michaelryanmcneill]

Didn't see your previous message. That was my recommendation and will be what I do to fix it. Sadly, I think we're a long way off from requiring PHP 7.x period.

[jrchamp]

If you want, here's a little cheat so that the value and constant keys are still defined:

return array( $value, $constant, 'value' => $value, 'constant' => $constant );

[michaelryanmcneill]

@jrchamp: do you think that it is worth it to add that cheat? My lean is that it is unnecessary.

[jrchamp]

It isn't necessary, but I was looking at the function docblock and it says "To get the value of the constant or option, look at the value key. To check if the value was retreived [spelling] from a constant, look at the constant key."

[michaelryanmcneill]

Ah good point. Yeah, probably best to reference anyways, just to be safe.