craue / CraueConfigBundle

Database-stored settings made available via a service for your Symfony project.
MIT License
173 stars 35 forks source link

Add setter to Config class #2

Closed jankramer closed 11 years ago

jankramer commented 11 years ago

In case you don't want to use a form to set the values

craue commented 11 years ago

Is it a good idea to immediately flush on each call? It would cause overhead if more than one setting has to be changed.

jankramer commented 11 years ago

True. I think you could either provide an extra (optional) boolean argument to set this behavior, or add a method in which you could update multiple settings at once.

craue commented 11 years ago

How about this?

/**
 * @param string[] $settings Names and values of settings.
 * @throws \RuntimeException If a setting is not defined.
 */
public function set(array $settings) {
    foreach ($settings as $name => $value) {
        $setting = $this->getRepo()->findOneBy(array(
            'name' => $name,
        ));

        if ($setting === null) {
            throw new \RuntimeException(sprintf('Setting "%s" couldn\'t be found.', $name));
        }

        $setting->setValue($value);
        $this->em->persist($setting);
    }

    $this->em->flush();
}

But it still has overhead of looking up each entity one by one. Not sure if there's a way to improve that.

jankramer commented 11 years ago

I updated the code, please have a look :) Also, you could perhaps run PHP CS Fixer on this project? It's not following the PSR-2 standard, or was this intentional?

craue commented 11 years ago

Indeed. I don't like that pseudo-standard.

Should we better call these methods update* instead of set* to not imply one could add new settings? Naming the argument $newSettings is also not a good idea.

I'll take a deeper look at the code later today.

craue commented 11 years ago

Just forget about my complaint regarding set/update, as it would be named add otherwise. So I'm fine with set now.

craue commented 11 years ago

Would you use tabs for indentation? Otherwise, I'll have to update all your code again. ;)

craue commented 11 years ago

Nice approach for fetching settings in setMultiple() btw. I guess that there's no way to also reduce the number of SQL UPDATE statements to only one for multiple settings. ;)

craue commented 11 years ago

Thank you.