fuel / helpers

FuelPHP Framework - Generic helpers library
MIT License
17 stars 7 forks source link

Config helper trait #7

Open sagikazarmark opened 10 years ago

sagikazarmark commented 10 years ago

Hence this package is for helper "classes", I see value in adding traits, like config trait with a config property and a getter/setter function. Would save some lines.

WanWizard commented 10 years ago

You mean a generic $config container?

The problem I have with this (and with the way it is implemented in a lot of classes) is that there is absolutely no validation whatsoever. Ideally, every config value should have a setter/getter, and if there is a generic method that accepts an array, it should only pass the data on to defined setters, so the input can be validated, defaults can be set, etc.

Which of course would mean no generic trait...

sagikazarmark commented 10 years ago

I absolutely agree with you, BUT until only option were these kind of implementations in fuel as well. So if there are no better solution then at least this could help the development.

I am not against moving in the direction of something more. Actually, I already have something similar implemented. An extension of DataContainer with Validation.

Or you can take a look at symfony option resolver which does something similar, but it gets immutable after first resolve.

WanWizard commented 10 years ago

Yes, I know. Validation takes time, which was one of the reasons v1 didn't do it (in most classes).

But I feel that if we specify a certain config key should be a boolean, at least we need to make sure it actually is one. It's not really something you can solve with a container type implementation, unless you can load it with a repository (but then you're very close to just using Validation). And it would not make the code any faster...

I've briefly looked at OptionsResolver, and it looks like it might do the trick. And it actually doesn't pull in half the Symfony framework as dependencies, so that is good too.

sagikazarmark commented 10 years ago

OptionsResolver is very flexible indeed, but there is a major problem with it: it gets immutable. So if you want to change any config key, you must validate the whole dataset again.

Also, IMO Validation gives you even more flexibility with it's rules. The only thing that is missing from validation is normalizer, but that is something we already discussed here.

Speaking of speed: yes, OptionsResolver is about ten times faster (tested it), but if you make a few sets, this difference becomes almost zero (because of the reasons mentioned above).

There is a problem with both solutions: nested config items are hard to be validated and Session, Email, etc use nested config.

WanWizard commented 10 years ago

Hmm... Bit odd that the entire validation runs on changing a single value. We could extend OptionsResolver to work around that?

Otherwise, can't validation be done through a callback? Then you can do what you want, including calling validation again on the individual items of the nested structure.

WanWizard commented 10 years ago

Yuck, it uses private properties...

sagikazarmark commented 10 years ago

What I could imagine with validation is that there is a Validator rule which contains a Validator instance and is run on nested properties. But that might be too much overhead.

Or just one collection rule with a set of other rules.