Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up getConfigItem #1196

Merged
merged 6 commits into from Sep 24, 2019
Merged

Clean up getConfigItem #1196

merged 6 commits into from Sep 24, 2019

Conversation

@jornane
Copy link
Member

@jornane jornane commented Sep 6, 2019

When using Configuration::getConfigItem or Configuration::getConfigList (only one user), the function can return a Configuration or $default. This makes it hard to handle default cases where the configuration settings are not in config.php.

This PR modifies these functions so that Configuration::getConfigItem only returns Configuration objects or null, and Configuration::getConfigList only returns arrays. This removes the need for return value checking in calling functions.

This happens at the cost of being able to require that the configuration option is set, but this is a feature; assume that ['assets']['cache']['max-age'] was required:

$assets = $config->getConfigItem('assets'); // returns empty Configuration
$cache = $assets->getConfigItem('cache'); // returns empty Configuration

// Throws exception that says that ['assets']['cache']['max-age'] is not set
$maxAge = $cache->getValue('max-age', Configuration::REQUIRED_OPTION);

In this PR, the return checking in the callers is also removed, and the unit tests are updated to reflect the new behaviour.

@jornane jornane requested a review from jaimeperez Sep 6, 2019
@jornane jornane requested a review from tvdijen Sep 6, 2019
lib/SimpleSAML/Auth/Simple.php Show resolved Hide resolved
Loading
@tvdijen
Copy link
Member

@tvdijen tvdijen commented Sep 6, 2019

This is great, but I'm hesitant to merge this until 2.0..
There are more cases like this, like Configuration::getPathValue that need work similar work..
I ended up doing horrible things like this to make sure I got a string back...

PS: did you check the modules for using these methods?

Loading

@jornane
Copy link
Member Author

@jornane jornane commented Sep 6, 2019

PS: did you check the modules for using these methods?

I don't understand much of the statistics modules, there's a lot of reflection there. But for the other modules, the behaviour should remain the same with this change.

Loading

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Sep 6, 2019

Oh you should check the mess it was around 1.13/1.14 🤣
I've already done so much work to rationalize that statistics-module! Twigifying it wasn't really a pick-nick either...

But for the other modules, the behaviour should remain the same with this change

That's great, but it could still mean a BC-break for modules out of our sight, that's why I said I am hesitant..

Loading

@jornane
Copy link
Member Author

@jornane jornane commented Sep 6, 2019

But for the other modules, the behaviour should remain the same with this change

That's great, but it could still mean a BC-break for modules out of our sight, that's why I said I am hesitant..

We can make requiring the default until 2.0, and then remove support for it at 2.0. That way, the function will behave exactly like it did before this PR, so no invisible modules breaking. That would be a light version of this PR.

The downside is that we ourselves have to add [] as the second argument everywhere, to not trigger an exception if the variable isn't set. We'd have to find those calls when moving to 2.0, but since it's only one function this may be done with a grep I guess?

However, I'm not really sure this would be a problem. Typically, your module will eventually call getString or getInteger on the config, which will throw an exception if the option is missing, which it is if you earlier got an empty Configuration object. The only case where this would fail, is where the final value is read with getValue, which is not something a module should do because you don't know what type you'll get back. Modules being hurt by this change are already doing dangerous stuff that may break. Personally, I think that this change is okay for SimpleSAMLphp 1.18, but not for SimpleSAMLphp 1.17.x.

Not doing anything and keeping getConfigItem like it is doesn't seem like a good option to me. Every time someone uses Configuration::getConfigItem, they're forced to write their own special-case-handling boilerplate around it. Since @jaimeperez wants to move towards a more hierarical config file format, the amount of Configuration::getConfigItem calls in the codebase is likely to go up.

Loading

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Sep 6, 2019

Not doing anything and keeping getConfigItem like it is doesn't seem like a good option to me.

I have many, many, many more of similar cases... I also like to move forward after talking about 2.0 for over five years.. We just can't break anything in a minor release, unless Jaime feels otherwise of course..
You could help us getting 2.0 closer though! Are you coming to the NorduNet TI this month?

Loading

@jornane
Copy link
Member Author

@jornane jornane commented Sep 11, 2019

@jaimeperez Some information on what behaviour changed:

Code that will keep working after this PR, but can be written shorter

// Example 1: required configuration

// BEFORE
$subconfig = $config->getConfigItem('foo');
// exception may be thrown stating that ['foo'] is missing
$val = $config->getString('bar');
// exception may be thrown stating that ['foo', 'bar'] is missing

// AFTER
$subconfig = $config->getConfigItem('foo');
$val = $config->getString('bar');
// exception may be thrown stating that ['foo', 'bar'] is missing
// Example 2: optional configuration

// BEFORE
$subconfig = $config->getConfigItem('foo', []);
$val = 'empty';
if ($subconfig instanceof Configuration) {
	$val = $subconfig->getString('bar', 'empty');
}

// AFTER
$subconfig = $config->getConfigItem('foo');
$val = $subconfig->getString('bar', 'empty');

Code that will break

// BEFORE (this is strange, but works)
$subconfig = $config->getConfigItem('foo');
$subconfig = $subconfig->toArray();

// AFTER (this will work both before and after this PR)
$subconfig = $config->getArray('foo');

Loading

@jornane jornane added this to the 1.19 milestone Sep 11, 2019
I guess this is personal, but I like phpdoc to be as explicit as possible
@tvdijen tvdijen merged commit 0e602c1 into simplesamlphp:master Sep 24, 2019
0 of 3 checks passed
Loading
@jornane jornane deleted the config-cleanup branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants