Forums

Unfortunately no one can be told what FluxBB is - you have to see it for yourself.

You are not logged in.

#1 2015-02-22 03:24:31

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Bad in system of addons

In calling any hook loads all addons! It is wrong.

I suggest to create a cache of such look:

$hooks_and_addons = array(
  'register_after_validation' => array('recaptcha.php'),
  'register_before_submit' => array('recaptcha.php'),
  'login_before_validation' => array('login_queue.php', 'security_of_login.php'),
  'login_before_header' => array('security_of_login.php'),
  'login_before_submit' => array('security_of_login.php'),
  'login_after_validation' => array('security_of_login.php'),
);

On the basis of this cache to load and initiate only those addona which are necessary.
The cache is created in case of its absence or if date of its creation less than a date of change of one of addon.

Last edited by Visman (2015-02-22 03:38:39)

Offline

#2 2015-02-22 09:14:17

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

I quote myself:

... Feel free to use some kind of "cache" to avoid traversing directories each time (the same way other caches are used in the admin panel) [...]

Another useful thing: instead of traversing through the addons directory, a automagical created "activatedAddons.php" could be used. This file only includes addons the admin enabled. Within admin panel you could then have your "addons traversal" and some simple [x] activate "ModuleName"-Checkboxes (previously activated ones should be preselected of course). Shouldnt be that tricky to code.
Of course this is not that needed, if we talk about 1-2 addons, but file access can be expensive, so traversing through directories should be avoided. The expense of a "activatedAddons.php" file access is of course to include in the consideration.
So to keep things "easy", such a file could be postponed to a later stage.

Src: https://github.com/fluxbb/fluxbb/pull/129


I hope I understood you correctly: You want to exchange the current "addon loader" with a cached one. Means recreating a cache file on changes and in all other cases loading only the cache file.

Ok, I proposed that too (not that "prominently placed" like your suggestion, but I did :-)). The easiest way is to have a "recreate addon cache"-button in the AP. Why?

The cache is created in case of its absence or if date of its creation less than a date of change of one of addon.

This would mean that the "addons.php" should check for file dates (mtime) of all addons - which is similar to including the files (less workload, but still working with many file handles).
Of course it could get semiautomated: the "homepage" of the admin panel could do that for us, think there are not that many admins out there in the boards to create that heavy struggle.

BUT ...as it still needs this route to go:
- addon modified/uploaded
- go to AP
-> addon cache is updated

we could keep the AP unclear about the addons system and just
- addon modified/uploaded
- go to AP
- go to addons-panel and press "recreate cache"
-> addon cache is updated


If we check for cache existence we could rely on:
we could keep the AP unclear about the addons system and just
- addon modified/uploaded
- delete cache/addons_cache.php
-> addon cache is updated on next forum render

Of course the last variant will lead to "lazy" admins forgetting things from time to time -> when uploading via ftp/scp/... you do not see the cache file in that moment (different directory).


So conclusion: mtime of files is only of interest for "auto-recreate cache" via AP-index. Missing cache should lead to recreation in all cases. Optional "addons"-recreate-button in the AP.


bye
Ron

Last edited by GWR (2015-02-22 09:19:43)

Offline

#3 2015-02-22 11:10:52

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

I wish that in each case only those addons are loaded, which in this case is used.
Cache only need to specify the boot loader which files need to be connected.
The method of comparing the time proposed for fully automatic implementation.

Offline

#4 2015-02-22 11:58:26

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

Like said, if you have a "fully automatic implementation" you still have to traverse through all addon files to check their modification dates (mtime).

This still leads to performance decrease compared to "no addon system".

That is why I suggested aboves variants all having advantages and disadvantages.

-> only semiautomatic systems could lower the load of the addons loader to a minimum of either a file inclusion (the cache file), a database query or an individual adjustment of the whole "addons.php" (which then has to be based on an addons_template.php which is filled with addon data during a creation function call).

bye
Ron

Offline

#5 2015-02-22 12:30:59

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

To check the date of creation / modification of files do not have to load them!?

Offline

#6 2015-02-22 14:14:24

chris98
Member
From: England, United Kingdom
Registered: 2013-05-31
Posts: 1,292
Website

Re: Bad in system of addons

Like said, if you have a "fully automatic implementation" you still have to traverse through all addon files to check their modification dates (mtime).

You could use filemtime to check when a file was last modified, and then something like format_time on that, which will display it properly for the currently logged in user. That way, it could be cached with it.

You could add it to the array Visman added, for example:

$hooks_and_addons = array(
  'register_after_validation' => array('modified' => '1424614396', 'file' => array('recaptcha.php')),
  'register_before_submit' => array('modified' => '1424614410', 'file' => array('recaptcha.php')),
  'login_before_validation' => array('modified' => '1424614421', 'file' => array('login_queue.php', 'security_of_login.php')),
  'login_before_header' => array('modified' => '1424614410', 'file' => array('security_of_login.php')),
  'login_before_submit' => array('modified' => '1424614421', 'file' => array('security_of_login.php')),
  'login_after_validation' => array('modified' => '1424614410', 'file' => array('security_of_login.php')),
);

The problem then would be updating the cache though...

Last edited by chris98 (2015-02-22 14:15:14)

Offline

#7 2015-02-22 14:56:00

Franz
Lead developer
From: Germany
Registered: 2008-05-13
Posts: 6,632
Website

Re: Bad in system of addons

Visman is, of course, right, that this could become a problem at some point.

I can't, however, imagine that this is already the case at this time. There are simply not enough large addons around.

That said, regarding caches: how about comparing the cache with the existing addon files only in the admin panel? Then we could display an alert to admins - something along the lines of: "Hey, you, there are some addons that should be activated (which happens by putting them in the cache)". Simple enough?


fluxbb.de | develoPHP

"As code is more often read than written it's really important to write clean code."

Offline

#8 2015-02-22 15:06:32

chris98
Member
From: England, United Kingdom
Registered: 2013-05-31
Posts: 1,292
Website

Re: Bad in system of addons

That sounds much cleaner - would a separate page be better for this? I think it would be easier putting them in the 'alerts' section of the dashboard, but then moderators might be able to enable them (unless stopped of course).

Maybe a better solution is to load it via AJAX and then it doesn't affect the rest of the page.

Last edited by chris98 (2015-02-22 15:08:47)

Offline

#9 2015-02-22 15:35:04

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

I will do my version of flux_addon_manager class. Then lay out the code here.

Offline

#10 2015-02-22 15:37:25

Franz
Lead developer
From: Germany
Registered: 2008-05-13
Posts: 6,632
Website

Re: Bad in system of addons

I'd love a pull request. smile

@chris98: I'd just do a simple alert (with a button), that is only displayed to admins.


fluxbb.de | develoPHP

"As code is more often read than written it's really important to write clean code."

Offline

#11 2015-02-22 15:39:30

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

@loading addons
No of course you do not have to load the addon files to check for "mtime" (or the php-functioncall returning it).

The problem in our case is an other type of "cache". File requests are cached. So you might have changed a file but for the upcoming php-script this file is still _not_ changed. Loading scripts might lead to a similar thing (if the whole board is running on APC or something in the likes) so this could get ignored (as both scenarios are affected by file system cache or APC/...).


@alerting in the AP
This is just another way to visualize the thing I already described in one of my aboves answers (especially #2).

As long as there is no "automatic addon update" there is no need to alert the admins - because the admins most of the times will be aware of the addon file they just updated ... I assume the one who is able to update addons is also capable of accessing the boards AP ...


@visman
I suggested an addon-list in the AP with [x]-checkboxes so admins could disable addons without removing them physically from the webspace. If you have enough spare time you might introduce that too.

- addon cache
- addon manager
- forcefully recreate cache! (in the case of board updates changing the availability of hooks)

BTW you could even extend the addon system to inform them about board updates so they could handle specific db updates on their own - but this is another story and might move them into a "full blown" solution we do not want for now.


bye
Ron

Last edited by GWR (2015-02-22 15:42:51)

Offline

#12 2015-02-22 15:48:30

chris98
Member
From: England, United Kingdom
Registered: 2013-05-31
Posts: 1,292
Website

Re: Bad in system of addons

I suggested an addon-list in the AP with [x]-checkboxes so admins could disable addons without removing them physically from the webspace. If you have enough spare time you might introduce that too.

If Franz approves, I could do this which would save Visman some work?

Offline

#13 2015-02-22 15:56:54

Franz
Lead developer
From: Germany
Registered: 2008-05-13
Posts: 6,632
Website

Re: Bad in system of addons

Don't pour too much work into this, please. We don't need a full-blown addon manager in the admin panel at this point, in my opinion.


fluxbb.de | develoPHP

"As code is more often read than written it's really important to write clean code."

Offline

#14 2015-02-22 16:11:26

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

If you do not want people to check/uncheck addons, you could even just create a dumb cache file and skip the sugar of an "alert: addons changed, recreating cache"-thing.

Concerning checkboxes: this is not that hard to implement so it wont steal that much of the others sparetime.

Nonetheless I understand your concerns (putting time and efforts into something which is designated to get replaced somewhen). But rethink: It is a "one time" job: do it once and it will just "work" but enhance convenience for admins by somewhat. It could be extended in the future so it just should contain basic functionality: enable/disable/configure/cache.
Configuration is already done via the addons on their own.

Splitting the whole addons into a subpage allows for addon names longer than the left side panel box tongue. Also it allows for addons limiting their configuration according to the user (by delegating the filter to the manager).
There might be addons for Admins and moderators (or addons which let things get customized according to the active moderator - eg a "dynamic per-post-signature" or whatever people might invent).

But as I am not writing that thing, I just accept with what you come up - I am aready satisfied with the "manually remove cache file"-approach tongue.


bye
Ron

Last edited by GWR (2015-02-22 16:13:07)

Offline

#15 2015-02-22 16:25:46

Franz
Lead developer
From: Germany
Registered: 2008-05-13
Posts: 6,632
Website

Re: Bad in system of addons

If we have a cache, I at least want the invalidation process to be somewhat comfortable (and performant), so the alert for admins seems like the perfect middle ground.


fluxbb.de | develoPHP

"As code is more often read than written it's really important to write clean code."

Offline

#16 2015-02-23 08:44:28

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

<?php

/**
 * Copyright (C) 2008-2012 FluxBB
 * based on code by Rickard Andersson copyright (C) 2002-2008 PunBB
 * License: http://www.gnu.org/licenses/gpl.html GPL version 2 or higher
 */

// Make sure no one attempts to run this script "directly"
if (!defined('PUN'))
	exit;


/**
 * Class flux_addon_manager
 *
 * This class is responsible for loading the addons and storing their hook listeners.
 */
class flux_addon_manager
{
	var $hooks = array();

	var $loaded = false;
	
	var $cache = array();

	var $loaded_addons = array();

	var $cur_file;
	
	var $create_cache = false;

	function load() // load all addons and create cache
	{
	  $this->loaded = true;
	  $this->create_cache = true;

		$d = dir(PUN_ROOT.'addons');
		if (!$d) return;

		while (($addon_file = $d->read()) !== false)
		{
			if (!is_dir(PUN_ROOT.'addons/'.$addon_file) && preg_match('%(\w+)\.php$%', $addon_file))
			{
				$this->cur_file = $addon_file;
				$this->loaded_addons[$addon_file] = filemtime(PUN_ROOT.'addons/'.$addon_file);
				
				$addon_name = 'addon_'.substr($addon_file, 0, -4);

				include PUN_ROOT.'addons/'.$addon_file;
				$addon = new $addon_name;

				$addon->register($this);
			}
		}
		$d->close();
		
		$content = '<?php'."\n\n".'$create_time = '.var_export($this->loaded_addons, true).';'."\n\n".'$cache = '.var_export($this->cache, true).';'."\n\n".'?'.'>';

		if (!defined('FORUM_CACHE_FUNCTIONS_LOADED'))
			require PUN_ROOT.'include/cache.php';

		fluxbb_write_cache_file('cache_hooks_and_addons.php', $content);
	}

	function load_hook($hook) // load addons for hook
	{
		if (!isset($this->cache[$hook])) return; // no addons for this hook

		$addon_files = $this->cache[$hook];

		foreach ($addon_files as $addon_file)
		{
		  if (isset($this->loaded_addons[$addon_file])) continue; // this file loaded

		  $this->loaded_addons[$addon_file] = true;

			$addon_name = 'addon_'.substr($addon_file, 0, -4);

			include PUN_ROOT.'addons/'.$addon_file;
			$addon = new $addon_name;

			$addon->register($this);
		}
	}

	function load_cache()
	{
		if (!file_exists(FORUM_CACHE_DIR.'cache_hooks_and_addons.php')) return false; // $this->cache = [], All addons are connected, load_cache() function will not be called more (!)

		include FORUM_CACHE_DIR.'cache_hooks_and_addons.php'; // variables $create_time, $cache

	  $this->loaded = true;

		$d = dir(PUN_ROOT.'addons');
		if (!$d) return true; // $this->cache = [], Addons are not connected, load_cache() function will not be called more
		
		while (($addon_file = $d->read()) !== false)
		{
			if (!is_dir(PUN_ROOT.'addons/'.$addon_file) && preg_match('%(\w+)\.php$%', $addon_file))
			{
			  if (!isset($create_time[$addon_file]) || filemtime(PUN_ROOT.'addons/'.$addon_file) > $create_time[$addon_file])
			  {
					$d->close();
					return false; // $this->cache = [], All addons are connected, load_cache() function will not be called more (!)
				}
				unset($create_time[$addon_file]);
			}
		}
		$d->close();

		if (count($create_time))
			return false; // $this->cache = [], All addons are connected, load_cache() function will not be called more (!)

		$this->cache = $cache;
		return true;
	}

	function bind($hook, $callback)
	{
		if (!isset($this->hooks[$hook]))
			$this->hooks[$hook] = array();

		if (is_callable($callback))
			$this->hooks[$hook][] = $callback;
			
		if (!$this->create_cache) return; // no create cache
		
		if (!isset($this->cache[$hook]))
			$this->cache[$hook] = array();
			
		$this->cache[$hook][] = $this->cur_file;
	}

	function hook($name)
	{
		if (!$this->loaded)
			if (!$this->load_cache())
				$this->load();

		$this->load_hook($name);

		$callbacks = isset($this->hooks[$name]) ? $this->hooks[$name] : array();

		// Execute every registered callback for this hook
		foreach ($callbacks as $callback)
		{
			list($addon, $method) = $callback;
			$addon->$method();
		}
	}
}


/**
 * Class flux_addon
 *
 * This class can be extended to provide addon functionality.
 * Subclasses should implement the register method which will be called so that they have a chance to register possible
 * listeners for all hooks.
 */
class flux_addon
{
	function register($manager)
	{ }
}

Offline

#17 2015-02-23 09:25:36

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

Before traversing addon script files and checking filemtime you might call "clearstatcache();" to remove potential caches regarding the file information.

Else script looks fine - maybe add some more comments (means: I had hard times to follow the basic structure of the script's "workflow").

Instead of

		if (!$this->loaded)
			if (!$this->load_cache())
				$this->load();

You could extend "load_cache()" to call "create_cache()" if the cache file is missing - and "create_cache()" is more or less the content of "load()".

So the workflow is:
- "hook()" it checks "loaded"
-- if not loaded, "load_cache()" is called
--- "load_cache()" checks for availability of the cache file, if that is missing "create_cache()" is called
--- else "load_cache()" will still check for an invalidated cache file (the mtime > cachetime thing)

bye
Ron

Last edited by GWR (2015-02-23 09:29:49)

Offline

#18 2015-02-23 10:07:54

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

Before traversing addon script files and checking filemtime you might call "clearstatcache();" to remove potential caches regarding the file information.

No, no and no!
When you upload files to the server via FTP also call clearstatcache ()?

At this time the cache is checked once. If no is cache, it is created and to save to the disk, but is not read again.
Why do I need to rewrite the code to re-read the cache file?

Offline

#19 2015-02-23 10:24:23

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

I do not know if ftp'ing clears file stats.

For me that command just clears php internal information about a file state. Most important if doing multiple checks on one file during one script execution (eg. "check filetime", "modify file", "check again" - this will need a clearstatcache() call).


@rewrite
You do not need to rewrite things at all. I just cannot follow the "workflow" of the script that easily. Might be the function naming or some logic ones, cannot explain that how I would do in my mother tongue.

In short: You have a function "load()". The only purpose of this function is what? to set "loaded" to true and to create a cache file. That cache creation should be done in an "create_cache()" function to make it more obvious what it does. I do not expect "load()" to create a cache ... but not check if it has to do.

So I just wanted to improve "readability" of your code. And for me that workflow is
- startup
- - load interested addons if not done
- - - is cache not existing or invalid, create cache
- - - use cache

I think your addon does that already pretty well, but somehow it was not easy for me to follow your script on what it does in which situation.


Why do I need to rewrite the code to re-read the cache file?

@re-reading cache files:
I did not say such a thing - nor did I want to express that.


EDIT: Check what I wrote in my earlier response... I wanted you to extend load_cache() to automatically run "create_cache()" if the cache is not valid or non-existent. Why? Because "load_cache()" for me sounds like a command, not a "boolean check" (like an "has_to_load_cache()" would imply). So when calling "load_cache()" I assume the cache to be filled at the end. Maybe everybody interprets this differently, so remember: everything I write here is "imho".

bye
Ron

Last edited by GWR (2015-02-23 10:28:22)

Offline

#20 2015-02-23 11:13:12

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

name function "load()" --> "create_cache()" and all ok.

Offline

#21 2015-02-23 12:03:15

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

Nope, "load()" adjusts "loaded = true", and "create_cache = true" - which seems a bit odd when the function is called "create_cache()".
This is what I meant with understanding the workflow. Your script is doing everything right - but the work is split into functions _I_ would do differently.

Currently it is as if "TCar.Prepare()" also starts the engine while I think "Prepare()" just adjusts mirrors, seats, puts the key into the key hole etc. .
I am sure others think like you and see no "problem" in your code, so forgive me creating this hassle. Like said: it is just my personal opinion and you are free to ignore it.

Let's wait what others say to your solution.


bye
Ron

Offline

#22 2015-02-23 13:28:33

chris98
Member
From: England, United Kingdom
Registered: 2013-05-31
Posts: 1,292
Website

Re: Bad in system of addons

I agree with Ron here. It seems a bit pointless and confusing that it's named load_cache and it really just changes boolean variables, that doesn't really seem to do much.

Last edited by chris98 (2015-02-23 13:29:41)

Offline

#23 2015-02-24 02:54:55

Visman
Member
From: Siberia
Registered: 2010-07-10
Posts: 1,200
Website

Re: Bad in system of addons

class addon_recaptcha extends flux_addon
{
    function register($manager)
    {
        if ($this->is_configured())
        {
            $manager->bind('register_after_validation', array($this, 'hook_register_after_validation'));
            $manager->bind('register_before_submit', array($this, 'hook_register_before_submit'));
        }
    }

incompatibility

Offline

#24 2015-03-02 17:46:21

quy
Administrator
From: California
Registered: 2008-05-09
Posts: 921

Re: Bad in system of addons

When creating the cache file, how about parsing each addon to determine hooks used and assign them to the their respective pages to the array?

$addons = array(
  'login.php' => array('login_queue.php', 'security_of_login.php', 'recaptcha.php'),
  'register.php' => array('recaptcha.php'),
);

Then in the load function, get the page name and only load the applicable addons from the array.

Offline

#25 2015-03-03 05:53:45

GWR
Member
From: Germany
Registered: 2010-08-06
Posts: 194

Re: Bad in system of addons

shouldn't it be

[EDIT]
Or just as Chris98 in post #6 already proposed.


Why? The code quy wrote does assume that a hook is only emitted from a single source. What about "error" hooks? (or "db error"-hooks etc.). So I think the "per hook name" association is a better approach.


bye
Ron

Offline

Board footer

Powered by FluxBB