Skip to content

Conversation

dcodadigital
Copy link

Changes to include test cases and code coverage compliancy from previous pull request #6.

Added in support for setting arrays
Not required. Originally added for testing purposes to compare
Configure::write against Setting::write.
Serializes arrays if used within Setting::write
General tidy up of code removing error suppression. Changes made
following phpunit tests as well as test cases added for testing
Setting::read/write arrays.
Applied compliancy for phpcs.
Conformed to phpcs.
Based on phpunit coverage report code has been updated to improve code
coverage in testing.
@bobmulder
Copy link
Contributor

@dcodadigital,

I see you had a lot of work on your code. Can you please provide me some explanation with code examples? What should your code do? Some pieces don't look very clean, however, appreciate your work ;)

Bob

@bobmulder bobmulder self-assigned this Oct 20, 2015
@bobmulder bobmulder mentioned this pull request Oct 20, 2015
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change to a lower version?

@bobmulder
Copy link
Contributor

@dcodadigital I added some comments. I really appreciate your work, but I really want it to be clean and the best, so I'm very strict in PR's. You've learned a lot, so go on! :)

@dcodadigital
Copy link
Author

@bobmulder I appreciate all your comments and thank you for taking the time to do that. As you may be able to tell I am new to github and code collaboration in general so even if this is not merged I have learnt a ton since last week. So thanks for being patient with me. :)

To explain what I was after achieving (I will submit inline comments after and make any updates if it's worth while for you) is I am using this on a project as a settings registry which it is perfect for that very reason and the only enhancement I needed for it to work with project was to be able insert and read arrays as well as insert into arrays in the same way Configure:: does for example:-

Configure::write('foo.firstname','Bob');
Configure::write('foo.lastname','Mulder');

Which when read

$foo = Configure::read('foo');

Which would give me both values in an array which is useful for me. So I was trying to apply the same idea in the same way so the following would be stored as 2 records with the arrays serialized as the value IE:-

Setting::write('foo.firstname','Bob');
Setting::write('foo.lastname','Mulder');

I also might need to fetch them individually as well which can be done by:-

Setting::read('foo.firstname');

Hope that makes sense and sorry this post has turned into a Charles Dickens novel.

@bobmulder
Copy link
Contributor

Hi @dcodadigital,

I like you think you are as good as Charles Dickens ;)

so even if this is not merged I have learnt a ton since last week. So thanks for being patient with me. :)

I've had that period as well (not a long time ago), so I completely agree by learning tons in a week and getting very much feedback ;)

I like your feature, and I think it should be added to the plugin, however it doesn't fit completely because of the naming that the plugin uses at the moment. At the moment this standard is used: PluginOrPrefix.SettingName. In your example you approach the Settings like the Configure class of CakePHP itself and I have to be honest, I think thats the right solution. So I agree by changing code to allow nested stuff / array's.

Next question is, does it fit in the current code or do we need to transform our whole structure and schema's to work with nested stuff?

A possible approach could be:

// writing this
Setting::write('foo.bar.firstname', 'Bob');
Setting::write('foo.bar.lastname', 'Mulder');

// should be saved in the table as

name value
foo.bar.firstname Bob
foo.bar.lastname Mulder

When you want to get data:

Setting::read('foo.bar.firstname');

// should return:
Bob

Setting::read('foo.bar');

// should return:

[
    'firstname' => 'Bob',
    'lastname' => 'Mulder'
]

Would like to hear what you think about this! I would like to have a chat with you! (https://gitter.im/bobmulder).

@dcodadigital
Copy link
Author

Hi Bob

I like you think you are as good as Charles Dickens ;)

If only! 👍 :-)

I've had that period as well (not a long time ago), so I completely agree by learning tons in a week and getting very much feedback ;)

I bet your thinking the same as me then why we did not go through the process even sooner. I've always just downloaded stuff of github and never really understood the collaboration aspect and how useful it is as to setting code standards and I think now it is a hurdle everyone should look to jump over.

Next question is, does it fit in the current code or do we need to transform our whole structure and schema's to work with nested stuff?

That is a good question I realize storing seralized arrays may not be the cleanest approach? The only advantage I can see of the serialized aspect from your examples which is a lot cleaner to the way that I approached it is if you need to have complex nested arrays as well as inter nesting arrays which would potentially require a lot of loops to separate them out by keys into the database? For me I am using a component which the app I am working on requires nested arrays for posttypes and taxonomies (shown at the bottom) which inter nest into one final array giving the posttypes and taxonomies for the project. So I think maybe mine is probably more of an isolated case use I'm not sure. What do you think?

examplecomponent
example-in-db

@bobmulder
Copy link
Contributor

May I ask you where you are working on? It looks like you are building a cms/admin-panel for Cake?

For your solution I would suggest the following: Just save your 'array' as string (json-encoded?) as setting. When you need data you get the string, decode it and its free to use. I think that's for you now the cleanest solution. If you want I can provide you an example.
On the other side, don't forget the settings plugin is only for storing small settings, not replacing stuff that should be an apart table! ;) I can imagine that you would like to store posttypes in an apart table and using a meta-table to store optional data (wich I am working on, so contact me for that progress).

About the general purpose, because settings are loaded every 'request' we have to be carefull by the amount of queries called. Note that complex queries (when you got a large application) will slow your application. So, that's why I chose for this strategy. We have to think about the main strategy: currently PluginOrPrefix.Name, why should we replace that? Only to allow nested stuff (like foo.bar.firstname)? We could easily implement a functionality to store and get arrays (so, getting foo.bar, but not foo.bar.firstname). What do you think about that?

I would like to have a chat with you! (https://gitter.im/bobmulder).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants