kovshenin / post-options-api

Post Options API is an alternative to Custom Fields
http://theme.fm/2011/08/post-options-api-an-alternative-to-custom-fields-1265/
57 stars 4 forks source link

Add meta boxes on "add_meta_boxes" hook, not "init" #4

Open tollmanz opened 13 years ago

tollmanz commented 13 years ago

I'm working with my second project utilizing post options API. On both projects, I called the function that initiates the post options API class from the "add_meta_boxes" hook. It did not work. Calling from "init" did.

The issue I have with this is that the "add_meta_boxes" hook was intended for the purpose of adding meta boxes. Since this project adds meta boxes, I think that it should work on this hook. Additionally, if the function that calls the post options API class methods is attached to the "add_meta_boxes" hook, that code will only be evaluated in the context of situations where meta boxes are needed. Using "init" forces WordPress to handle this code on each request.

Currently, I cannot figure out why "add_meta_boxes" won't work. I'm going to look into it more. Any thoughts?

kovshenin commented 13 years ago

@tollmanz, this is quite tricky actually. Your thinking is correct but take the get_post_option method for example, that could be used anywhere, even outside the admin. Of course at it's current stage it's simply a wrapper around get_post_meta but there may be additional things going on there at some point.

Anyhow, moving the actual metaboxes creation (not the whole class) to add_meta_boxes hook is a good idea and I'll probably do that, but your initialization and options/sections registration should really happen before that :) Thoughts?

tollmanz commented 13 years ago

@kovshenin...Yeah, I get that "get_post_option" won't work if not added early on. I've not been using it because I don't see how it's any better than "get_post_meta" at this point. I can see that down the line, this could be a more powerful and useful function and it would be good to be forward thinking about how it would be implemented.

Looking at this now, I think that the only method in the "Post_Options_API_1_0_1" that would need to run earlier than "add_meta_boxes" is the "_save_post" method (but I'm not entirely sure about that).

My main concern is that on something like a front end request, the "Post_Options_API_1_0_1" class does not need to be instantiated. I think it would be good form to relegate it to only admin requests, and even better, to only situations when meta boxes are needed.

kovshenin commented 13 years ago

@tollmanz, I ran a quick benchmark, I started a microtime before calling require and ended it after the last register_post_option call in the plugin example. Execution time is 1.28 milliseconds :)

Anyhow, I do understand your concerns and it's probably a matter of design and code quality rather than performance (at least at this stage). What do you think of introducing an action, like register_post_options to sort of lazy-load the options registration? Then you could squeeze your registration codes in there and not worry about when it's going to be carried out -- when the meta boxes are loaded or if they're not when the first call to get_post_option is issued.

Needs to be thought through and tested because of the current compatibility scheme. We don't want two different bundled version to register options twice, right? :)

tollmanz commented 13 years ago

@kovshenin, would love to see what you have in mind. Unfortunately, I not sure how you would implement the lazy load option, but would like to see it in action.

tollmanz commented 12 years ago

This might be a bigger issue than I thought. I'm seeing a nasty side effect of initiating the meta box functions on init instead of add_meta_boxes. I've registered 9 fields across two sections. When I go to change a post via quick edit, I get a Fatal error: Allowed memory size error. It's completely reproduceable and the memory is exhausted in the exact same place each time.

Initially, I had 8 fields in one section. Everything worked fine. Once I added a second section with one field, this error began. This is just a place that these functions should not run. I'm going to look into a way of changing this.

kovshenin commented 12 years ago

@tollmanz, can I take a look at the code that you're using and what is your memory limit in php.ini? Perhaps I'm missing some "cleanup" scenario but I don't think it's related to init, memory's consumed and it doesn't really matter where, you'll probably still get a fatal whether it's init, admin_init or even wp_footer :)

tollmanz commented 12 years ago

@kovshenin, sorry for the late reply. Unfortunately, I've moved past this issue and don't think I have the code anymore. I'll try to dig it up.

I agree that the issue will likely still occur with those hooks, but again, shouldn't we be loaded the library only when necessary. The site I was working on needs to perform really well. It's a problem if the library is getting loaded on every page view even when it's not needed. I understand that parts of the class need to be loaded at other times, but I just am not liking the overhead of loading this library on every request.

kovshenin commented 12 years ago

@tollmanz, late reply me too, yeah I know :) I'll first sort out the memory problem and then look for better place to load up the stuff. Any luck finding that code again? P.S. What is the memory limit set to on your setup?