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

Config helper trait #7

Open
sagikazarmark opened this issue Jun 10, 2014 · 7 comments
Open

Config helper trait #7

sagikazarmark opened this issue Jun 10, 2014 · 7 comments

Comments

@sagikazarmark
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

Yuck, it uses private properties...

@sagikazarmark
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants