OUGC Pages Review

[ZiNgA BuRgA]

This "review" was requested by Sama34.

Rather late with this one, but I managed to get around it.  I haven't been doing PHP coding much lately, so my memory is a bit weak, meaning that I've likely glossed over much more than I usually would do.

Anyway, the plugin generally seems to be fine.  Some suggestions I'd make:

• It's probably not clear what one needs to exactly do to integrate Pages into the forum.  By default, no links are added.

• PHP Code:

  $is_page = $mybb->get_input('page') && !empty($mybb->cache->cache['ougc_pages']['pages'][$mybb->get_input('page')]);

  ---

  Variable not used anywhere, consider removing.

• PHP Code:

  $query = $db->simple_select('ougc_pages', 'pid, url', 'visible=\'1\''.(!empty($update['categories']) ? ' AND cid NOT IN (\''.implode('\', \'', $update['categories']).'\')' : ''), array('order_by' => 'disporder'));

  ---

  The attempt to filter out categories doesn't make a lot of sense, since the rest of the code seems to assume that all pages are stored in the cache.
  The filter doesn't do anything anyway - I assume that the implode part was supposed to be
  

  PHP Code:

  implode('\', \'', array_keys($update['categories']))

  ---

  (though this obviously breaks things)
  My guess is that it's legacy code that forgot to be removed.

• The categories cache (MyBB level cache, not the cache that is tied to the class) doesn't appear to be used.  Perhaps if it was keyed by URL, like the pages cache, it could be more useful.
• My mistake with this one:
  
  PHP Code:

  // convert \xA0 to spaces (reverse  ) $message = trim(preg_replace(array('~ {2,}~', "~\n{2,}~"), array(' ', "\n"), strtr($message, array("\xA0" => ' ', "\r" => '', "\t" => ' '))));

  ---

  "\xA0" should be utf8_encode("\xA0")
  (though I can't actually see this code being used anywhere in the plugin - my guess is that it's just a generic include across all OUGC plugins?)

• I personally don't agree with stuffing all the code into the ougc_pages.php file.  For example, the ougc_pages_show function is only ever called from pages.php, so it may as well be placed there so that it doesn't need to be loaded on every page load.  I suppose this is more of a design decision though.
  

[nier3]

Thank you for review! I'm looking for a plugin to easily and quickly add many custom pages, without create php page each time. I'll try it, thanks

[Sama34]

Quote:It's probably not clear what one needs to exactly do to integrate Pages into the forum.  By default, no links are added.

There I'm once again assuming things, since I do use the Global Menu plugin by RateU to manage my own links and I expect some minimum understanding on the template system and therefore HTML I never though about this. I will probably add a feature to do this less manually.

Quote:The attempt to filter out categories doesn't make a lot of sense, since the rest of the code seems to assume that all pages are stored in the cache.

Hidden categories should be not visible nor its pages, that is why I only intend to cache visible pages within visible categories. Though yeah the implode fortunately seems to be wrong because from a quick look the query should be an 'IN' not 'NOT IN', haha..

And yeah, the categories cache is not used even when that was the plan. Unsure about the reason though.

Quote:my guess is that it's just a generic include across all OUGC plugins?

Correct. I will need to update all of my plugins that use that function at some point. All of them use the same named function...

Quote: personally don't agree with stuffing all the code into the ougc_pages.php file.  For example, the ougc_pages_show function is only ever called from pages.php, so it may as well be placed there so that it doesn't need to be loaded on every page load.

This was to return a value instead of directly printing the page to use within the portal. I removed this functionality before releasing because of lack of time for testing it. Commented code can be seen within all the script.

Thanks for your review Zinga Burga.

[ZiNgA BuRgA]

Ah I see.
Thanks for the reply.