JoeSz / Exopite-Simple-Options-Framework

Fast, easy and lightweight admin options/metabox form generator for WordPress Plugins and metaboxes.
GNU General Public License v3.0
78 stars 24 forks source link

Dashes in Filter and Action names #5

Closed raoabid closed 6 years ago

raoabid commented 6 years ago

The WordPress PHP Coding Standards Handbook states:

Use lowercase letters in action and filter names. Separate words via underscores.

Reference: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#custom-word-delimiters-in-hook-names

The filter and hook names used in the framework have dashes in them. I would recommend to replace the dashes with underscores.

To remain backward compatible, we can also use underscored hook names alongwith dashed hook names.

For instance,

The current hooks are like this:

// Filter for override
$this->config = apply_filters( 'exopite-simple-options-framework-config', $config );
$this->fields = apply_filters( 'exopite-simple-options-framework-options', $fields );

We can update the code to:

// Filter for override
// TODO: filter name 'exopite-simple-options-framework-config' shall be removed in future version, use 'exopite_simple_options_framework_config' instead
$this->config = apply_filters( 'exopite-simple-options-framework-config', $config );

// Revised filter name
$this->config = apply_filters( 'exopite_simple_options_framework_config', $config );

// TODO: filter name 'exopite-simple-options-framework-options' shall be removed in future version, use 'exopite_simple_options_framework_options' instead
$this->fields = apply_filters( 'exopite-simple-options-framework-options', $fields );

// Revised filter name
$this->fields = apply_filters( 'exopite_simple_options_framework_options', $fields );

This is how we can update the existing code and remain backward compatible.

I guess this is the time too make this change as the framework is still in its infancy.

Thoughts are welsome

JoeSz commented 6 years ago

I agree. Please do the corrections if you want to or I will do if I have time :)

Backward compatible:

For "meta" nothing changed, there is no need for that.

I think like this is better than disable $current_lang, because in this case, developer will think about multilang. So maybe there is a good point to remove "old" filters.

What do you think?

raoabid commented 6 years ago

Do we have any idea as to how many developers using this in production?

i am seriously thinking of giving "menu" and "meta" their own classes. or may be these classes can abstract a main class.

but making separate classes for menu and meta will make the code modular and will keep things organized.

for instance, we make classes like: Exopite_Simple_Options_Framework_Meta and Exopite_Simple_Options_Framework_Menu

But i will have to explore this possibility more.

If the developers using this framework are not much, we can simply go ahead and change the hook names instead of worrying about backward compatibility.

JoeSz commented 6 years ago

I think not too many developers using this in the momment.

But I think separate them is not nessesearly a good idea, because I put them together on purpose :)

If you see CodeStar, there those are in the separate classes.

It is not so many differents between them and it is easier to maintain the code like this in my opinion.

Maybe could be more comments or somehow separate them a little bit more, but in the moment I want to keep them together.

raoabid commented 6 years ago

Ok, We are not going to separate the classes. if backward compatibility is not yet needed, i can simply change the filter names. Need your approval for this.

or should i watch out for backward compatibility? (it will add more hooks calls during the load of plugin options or CPT page.

JoeSz commented 6 years ago

I think with my new multilang compatibility, backward compatibility will be broken. It is possbile to convert the options array forth and back through. I will create a new branch for the multilang changes, you could take a look if you want. But I need to review first your changes and see, if it is still compatible with my or should I make some corrections. I try to do this in the next few hours, but I can not promise :S

Please change the filter names. Now it is a lot of changes and I do not want to force backward compatibility on the project in the momment, because

JoeSz commented 6 years ago

The Exopite_Simple_Options_Framework_Upload::add_hooks(); is for both (meta and menu) and $i++ should be $i++ not $i ++ (space between), am I right?

raoabid commented 6 years ago

$i++ and $i ++ are same. noted the requirement of Exopite_Simple_Options_Framework_Upload::add_hooks(); for both.

raoabid commented 6 years ago

This issue was resolved in pull request #7