Some (optimization) & feature suggestions

[Kirby]

In AdminCP, input values are often cleaned twice (calling clean_array_gpc() and clean_gpc(), this seems inefficient
Action switches are done via in_array() calls. This seems inefficient as array-operations are costly. I'd suggest to use if ($_REQUEST['do'] == 'action1') OR $_REQUEST['do'] == 'action2')
You are doing an opt-in check for checking view permissions on pages. Assuming that most pages are publically visible, this seems inefficient - "opt-out" might be better.
Also, you are emulating an array-intersectin in native PHP. I think using array_intersect() instead would be faster.
The page navigation module issues a query to get all pages and build the necessary data structures. It might be worth considering a cache here
It would be nice to have a "Add Page" action in the page listing in ACP, so one can easily add sub-pages (parent set automatically)
The page navigation module only allows to add additional links after all pages, it would be nice if additional links could be freely placed as well
An additional page type "HTML content" with FCKEditor or TinyMCE included would be nice. This way we could add easily content - without overhead in templateidlist, for bbcode parsing or stat'ing files.
WOL support; controls in ACP to enter description/link for the pages
Option to display some kind "page not found" error if a page does not exist
Allow modules to be freely paced on pages and not be bound to columns
Better yet: Get rid of the three columns alltogether and let the user define locations (the default could still be left, middle, right).
This way the user can create a layout as he likes :)
Hooks :) in several strategic places in ACP
Some kind of collapsing for the page list in ACP, so we can hide sections (like in Forum Manger) and not have a loooong list of pages

[BrentWilson]

WOL support; controls in ACP to enter description/link for the pages

This would be NICE!

[Kirby]

More suggestions ;)


Make use of the product system; use module identifiers as productids
This way phrases/templates could be kept in master style without problems
Mass-Update Module configuration
A control in ACP where we can select multiple pages, module configuration and update them


If NE1 else except me is interested in all those changes/features, I might be able to provide a patch as I am going to implement most of them anyway.

[BrentWilson]

More suggestions ;)


Make use of the product system; use module identifiers as productids
This way phrases/templates could be kept in master style without problems
Mass-Update Module configuration
A control in ACP where we can select multiple pages, module configuration and update them


If NE1 else except me is interested in all those changes/features, I might be able to provide a patch as I am going to implement most of them anyway.
I am :)

[1QuicksSI]

Me too :) Though someone from here is going to have to help me since this thing wrecked my development site and I have been down now for 3 days :(

[Brian]

Thanks for the suggestions Kirby. :)


In AdminCP, input values are often cleaned twice (calling clean_array_gpc() and clean_gpc(), this seems inefficient
Thanks for pointing that out. I remember when 3.5 first came out I was (obviously) confused as to how the clean_gpc() functions and such worked. I realized this later, but apparently forgot to remove these from the CMPS admin page.

Action switches are done via in_array() calls. This seems inefficient as array-operations are costly. I'd suggest to use if ($_REQUEST['do'] == 'action1') OR $_REQUEST['do'] == 'action2')
True, but I haven't been that worried about it since it's in the admin page and isn't viewed often, plus the in_array() function just makes it a little easier than listing them out. And considering it's not a main page that is viewed often, I would think the performance hit would be so insignoficant that it wasn't worth worrying about.

You are doing an opt-in check for checking view permissions on pages. Assuming that most pages are publically visible, this seems inefficient - "opt-out" might be better.
Actually it is done that way for a reason. I would think the main concern for things such as this should be security and privacy. Currently, if you have a page only available to a certain group(s) and you add a new usergroup in vB, this new group will not gain access to the private page. If the permissions were changed to an opt-out system, then this new group would gain access to the page inadvertently if you forgot to go back and change your page permissions. I would think this would be a worse scenario than a new group not having access to a page.

Also, you are emulating an array-intersectin in native PHP. I think using array_intersect() instead would be faster.
For the permissions section? Could you point out which code? I took a quick look, but I must be missing something.

The page navigation module issues a query to get all pages and build the necessary data structures. It might be worth considering a cache here
That's more along the way I was planning on doing things originally, but probably opted for the small query to save a little time until another release. This wasn't on my current to-do list though, so thanks for reminding me. :)

It would be nice to have a "Add Page" action in the page listing in ACP, so one can easily add sub-pages (parent set automatically)
Good idea.
*Adds to to-do list*

The page navigation module only allows to add additional links after all pages, it would be nice if additional links could be freely placed as well
This is a planned feature, but may take a version or two before it's included since it will take just a little work to make this function the way I would like it to.

An additional page type "HTML content" with FCKEditor or TinyMCE included would be nice. This way we could add easily content - without overhead in templateidlist, for bbcode parsing or stat'ing files.
Never heard of those before. I've added checking them out to my to-do list though. :)

WOL support; controls in ACP to enter description/link for the pages
It hasn't been added yet so I can't say to what extent it will cover, but WOL support is planned for the next version.

Option to display some kind "page not found" error if a page does not exist
Shouldn't take long to add in, so I've added a note to make an option for that.

Allow modules to be freely paced on pages and not be bound to columns
Better yet: Get rid of the three columns alltogether and let the user define locations (the default could still be left, middle, right).
This way the user can create a layout as he likes :)
Eventually the whole 3 column layout will be done away with for a much better/more complex system, giving the user much more control. That's another thing that is going to take some time though, so I can't make any promises as to which version that will be included in.

Hooks :) in several strategic places in ACP
Could you please elaborate a little on this?

Some kind of collapsing for the page list in ACP, so we can hide sections (like in Forum Manger) and not have a loooong list of pages
Hadn't really thought about that before. I'll see what I can do though. :)

[Kirby]

My site gets 10+ million PI/month, so I am pretty keen on optimizing everything and those were all things I came across :)
All my pages are public, so there is no real need for permission checks - for me.

Maybe a switch for the permission system: Opt-In/Opt-Out/Disabled?
This would cater everyone; those who need privacy as well as "big" sites that need performance.

I've implemented a big chunk of the stuff I've mentioned in my previous posts (except freeing columns and WOL, will do thin in the next days I guess).
If you are interested, I can send you my version.

For the permissions section? Could you point out which code? I took a quick look, but I must be missing something.

_top.php

// Get the user's groups
$vbulletin->userinfo['usergrouparray'][] = $vbulletin->userinfo['usergroupid'];

if ($vbulletin->userinfo['membergroupids'])
{
foreach (explode(',', $vbulletin->userinfo['membergroupids']) AS $groupid)
{
$vbulletin->userinfo['usergrouparray'][] = $groupid;
}
}

$allowview = false;

// Check user's groups against page permissions
if ($pages['userperms'])
{
$pageperms = explode(',', $pages['userperms']);
foreach ($vbulletin->userinfo['usergrouparray'] AS $groups)
{
if (in_array($groups, $pageperms))
{
$allowview = true;
}
}
}


This could be shortened to

// Get the user's groups
$vbulletin->userinfo['usergrouparray'] = fetch_membergroupids_array($vbulletin->userinfo);

$allowview = false;

// Check user's groups against page permissions
if ($pages['userperms'])
{
$pageperms = explode(',', $pages['userperms']);
$allowview = (array_intersect($vbulletin->userinfo['usergrouparray'], $pageperms)) ? true : false;
}


which should theoretically be faster, as it uses a built-in PHP function rather than emulating it. :)

And more stuff I've come across

forum permissions are always built, the parser is always loaded & instatiated
If you mainly have HTML based content pages, this is unnecessary CPU und memory overhead.
I made two singleton functions to init those when needed (eg. the modules have to call init_forumperms(), init_parser() when they need them)
There is no way to cache templates depending on certain actions.
This is kinda bad if you have a complex modules that use a lot of Templates, but not always all of them - even worse if it uses the editor.
Some possibility to enter init-code for a module would be useful, or a function init_module_{identifier} that could cache templates/phrasegroups.
Please use valid product identifiers for templates/phrases and put them in the master sets - it makes it sooo much easier :)
The default page identifier 'page' is kinda nasty if your modules use construct_page_nav() ...
Use phrases for module settings

[rushwhq]

hi all

where is _top.php ?

[Kirby]

In includes.

[XtAzY]

lol if only kirby and brian can team up and work together and release a ultimate portal system XD
having two people workin together might bring this cmps to be the best~!

any yes i wish the navigation system have more improvements... cuz i wish the navigation can do like this: show Subforum titles, or more navigation modules that dunt repeat the same pages as used in the other navigation

[Kirby]

Some more suggestions:


The stats module queries the user table for the top poster which causes a full table scan - bad.
As this data changes rarely, what about using a cache here?
Could be updated with a cronjob once every (week/month/whatever) or update it (if necessary) when a new post is made.
The welcome block module causes some heavy scans on the post table when displaying the amount of new posts since last visit.
What about this approach:

A new column newposts in Table session (default NULL)
When the welcome block module is being displayed for a user and this field is NULL - update it with the number of new posts; otherwise just display its value
When a new post is made, UPDATE session SET newposts=newposts+1 WHERE NOT ISNULL(newposts)

[Gary Bolton]

Very nice Kirby, I also hope Brains takes you onboard to use your knowledge on improving vBa were it can be improved, optimimaztion is a real must these days and some of your ideas seems really good

[Brian]

A good bit of Kirby's suggestions have already been implimented in the dev files. His last two *may* be implimented in the next version, but I will have to see how much time I have to work on things.

[DementedMindz]

If NE1 else except me is interested in all those changes/features, I might be able to provide a patch as I am going to implement most of them anyway.

i would love if you would provide patchs for this i have used your one you posted thanks if you know of any others please provide :)

[Kirby]

- Please make the cpnav XML attached to the product
- Don't use addslashes() as it has issues => $db->escape_string()
- Use Class vB_Database_Alter_MySQL to alter tables (add indexes, etc.)
- the require for the newpost plugin isn't really worth it, put those 3 lines streight into the plugin

[Kirby]

OK, the following is a bit hacky, but it could give a fair performance boost:

If the user just want's to display the latest poll (eg. the last one created), you could use smth. like the following query


SELECT thread.pollid, open, threadid, replycount, forumid, question, poll.dateline, options, votes, active, numberoptions, timeout, multiple, voters, public
FROM poll AS poll
INNER JOIN thread AS thread ON (thread.pollid=poll.pollid)
WHERE open <> 10
AND visible = 1
AND active = 1
AND poll.pollid=(SELECT MAX(pollid) FROM poll)

maybe with more conditions to check forumids, etc.

This query is almost O(1), as it should not cause any table scans.
In case it does not return data (I'd say in 90% of all cases it should return what's needed), you could use the current query as a fallback.

Would also require to check if mySQL is at least 4.1

[GITs]

it would be nice to be able to define how many colums each module spans across, so you could have a single module span the whole of the three columns or two or whatever.. would be a nice touch and would integrate the modules into each other better.