17 February 2011, 15:55

Sounds really good to me Ruslan, the only problem is that we can't really break the smarty function because too many non-core plugins will be using it.

But maybe this is a good time to write a dwoo() function, that takes a sensible, less ugly set of arguments, and leave smarty() there, but change it into a wrapper around dwoo() that sets the appropriate globals.

It'd also be a good time to fix one other thing that's always bugged me, adding some kind of warning whenever a programmer calls pieform() after the dwoo/smarty function was already called.  Sometimes that means the $GLOBALS['_PIEFORM_REGISTRY'] isn't set, so the header js doesn't get included, and the form doesn't work.

18 February 2011, 5:33

Hi Richard,

> the only problem is that we can't really break the smarty function because too many non-core plugins will be using it.

I did not mean breaking it, rather changing the way how header related stuff is defined, which will make it cleaner and preserve all existing functionality, i.e.

$stringjs = '<script type="text/javascript">';
$stringjs .= 'var strings = ' . json_encode($strings) . ';';
$stringjs .= '</script>';

will become

$HEADER->add_js_string('var strings = ' . json_encode($strings) . ';')

another example

$stylesheets = array_merge($stylesheets, array_reverse($pluginsheets));

will become

$HEADER->add_css_link(array_reverse($pluginsheets));

Your dwoo function idea will basically suggest the same but another way, through creating a proper dwoo function with all new stuff and turning smarty as a wrapper for it, which sounds good. Shall we aim this to 1.4 then?

20 February 2011, 16:37

That sounds excellent.  Do you think it might be helpful to have some kind of optional weight passed into add_js_string, add_css_link so that plugins can specify css priorities or js dependencies?

21 February 2011, 3:25

Pririties sounds interesting, but I can't think at the moment where they might be useful. Can you give examples? Regarding dependencies, it probably worth adding a parameter where dependencies will be listed, though it will also be possible to add all required files with add_js_file method.

21 February 2011, 14:54

I'm thinking of all the crud that's currently sitting in the smarty function that tries to make sure tinymce goes in first, jquery goes in before mochikit, rtl.css goes after all the other css files, stuff like that.  Might be nicer if you could look at some numbers and see at a glance what order they were going to be put in to the head.

22 February 2011, 3:07

Right, it makes sense then adding the weights as parameters. I will soon update the branch I initially requested to merge with changes we discussed and let you know that it is ready to be revised/merged ;)

13 December 2010, 1:10

And since you asked for specific feedback about security and performance in your implementation, I should point out two things:

1- the "if (table_exists(...))" call is unnecessary because that's already checked in plugin_types_installed().

2- the "safe_require(...)" call may need to be cached via a static variable trick to avoid stat'ing each file more than once on every page load.

Security-wise, I didn't see anything wrong. Of course, any extension taking advantage of these hooks needs to be fully trusted, but that's obvious, being PHP code.

Cheers,

Francois