hook not triggered / integrate_update_settings_file

Started by danielrichter, December 31, 2021, 02:49:24 AM

Previous topic - Next topic

danielrichter

Running SMF 2.1 RC4, a question came up.

Any idea why the hook integrate_update_settings_file won't be triggered?

hook.php:

<?php
if (file_exists(dirname(__FILE__) . '/SSI.php') && !defined('SMF'))
require_once(dirname(__FILE__) . '/SSI.php');
elseif (!
defined('SMF'))
die('<b>Error:</b> Cannot install - please verify you put this in the same place as SMF\'s index.php.');

add_integration_function('integrate_pre_include''$sourcedir/Subs-YourAction.php');
add_integration_function('integrate_update_settings_file''youraction_add_hook'false);

die(
var_dump($cache_redis));
?>

/Sources/Subs-YourAction.php

<?php
if (!defined('SMF'))
die('Hacking attempt...');

function 
youraction_add_hook(&$settings_defs)
{

$file fopen("/var/www/html/forum/test.txt""w") or die("cannot open");
$txt "test\n";
fwrite($file$txt);

$settings_defs["cache_redis"] = "bla";
}

?>

Even the "log" part isn't triggered (file permissions are just fine):

Quoteroot@dev-clus1:~# getfacl /var/www/html/forum/test.txt
getfacl: Removing leading '/' from absolute path names
# file: var/www/html/forum/test.txt
# owner: www-data
# group: www-data
user::rw-
group::rwx
other::r--

Seems like the hook is never triggered.

Aleksi "Lex" Kilpinen

#1
The up to date codebase is available at https://github.com/SimpleMachines/SMF2.1 ( Edit: To clarify, this was to answer something that is no longer included in the OP. ) As for the other question, you will have to wait a bit for someone more knowledgeable to come along.
Slava
Ukraini!


"Before you allow people access to your forum, especially in an administrative position, you must be aware that that person can seriously damage your forum. Therefore, you should only allow people that you trust, implicitly, to have such access." -Douglas

How you can help SMF

Arantor

-sigh- I wish I'd never replied to the other topic to be a starting point because this is exactly what I feared. But to avoid this drowning otherwise in a sea of irrelevance and in the hopes it might be useful to someone who can actually make use of it...


OK, so first problem: a fundamental misunderstanding of what hooks are and how they work. As I said before, a hook is 'I am at this point in the code, any addons want to do anything?'

Specifically in this case, integrate_update_settings_file is a point part way through the updateSettingsFile routine in Subs-Admin.php. It only ever gets called *when updateSettingsFile gets called*. Which is when various parts of the admin panel will change the settings file, such as (but not limited to) the cache settings.

And seeing that you have more than one page of the admin panel that can arbitrarily change the Settings.php file I think you're going to have real fun with that in your multi-container architecture but that's another story entirely.

Anyway, the updateSettingsFile routine is very complex, very densely written and is mostly about reconstituting the Settings.php file entirely on demand because of historical issues where the file can be erased by a race condition. As a result you need to make sure that $cache_redis is an entry in that list so when *any* routine calls updateSettingsFile, $cache_redis will be taken into account. If not, there's a very realistic possibility that your cache settings will just be dropped. This is why you have to use this hook to modify existing behaviour.

This means, furthermore, that you're calling the integration functions backwards; you don't want to set the 'permanent' to false as you have, you actually want it the other way around. The mod installer should call add_integration_function with true so that it's actually set not just for the rest of the page load but for all future page loads.

In fact what you really want to do is call it slightly differently. The function signature:

function add_integration_function($hook, $function, $permanent = true, $file = '', $object = false)
$hook is of course 'integrate_update_settings_file', $function is the name of your function that you want to call during updateSettingsFile, $permanent should be true so it will call on future calls (not just the life of the current request).

Now, you could put a separate file in for $file, but you shouldn't have to because this can be much, much tidier. You can skip it entirely and leverage SMF's internals better.

When you put in Sources/Cache/APIs/Redis.php, you could add a static method to that class just for the settings file hook, maybe let's call it integrate_update_settings_file as a method, so SMF\Cache\APIs\Redis::integrate_update_settings_file would be a static callable that does the relevant array manipulation needed, and then you would be able to put that as the function in the add_integration_function call (as a fully qualified callable name).

The entire SMF\Cache namespace is autoloaded looking at index.php so that should be fine to just reference by name and not have to explicitly load it any other time. No point adding a file that you don't need to load.

Yes, I know it isn't documented properly, that's because the integration documentation looks like it was lifted from my post a decade ago and hasn't been updated since for all the 2.1 specific changes.

I still don't entirely believe you *need* caching and could just turn it off because I don't believe that a new forum will ever have the traffic to require caching in the first place.
No good deed goes unpunished / All helpful urges should be circumvented

I have something to say: it's better to burn out than to fade away. There can be only one.

Sesquipedalian

Quote from: Arantor on December 31, 2021, 02:07:20 PMIf not, there's a very realistic possibility that your cache settings will just be dropped.

This is technically correct, but the following clarification might be helpful here:

Under normal circumstances, a setting would never be dropped by updateSettingsFile(), regardless of whether it has been given a definition in the $settings_defs array. Instead, a setting without a definition in $settings_defs will simply be handled as a custom setting: the first time it is added, it will inserted into Settings.php just before the error catching code near the end of the file, and then in subsequent updates to Settings.php it will be preserved as is.

The only time a custom setting could conceivably be dropped would be if all the following occur:

  • The $config_vars parameter passed to updateSettingsFile() does not currently contain the custom setting.
  • Settings.php becomes corrupted sometime after the current SMF execution has read it, but before the current execution has completed.
  • Either:
    • Settings_bak.php is not corrupted but does not contain the custom setting.
    • Settings_bak.php is also corrupted and $settings_defs does not contain a definition for the custom setting.

The reason why $settings_defs matters in condition 3.2 is that, if both Settings.php and Settings_bak.php are corrupted, then updateSettingFile() tries to recover by using $settings_defs as a guide to rebuild Settings.php on the fly.

Apart from that emergency situation, however, a $settings_defs entry is more of a "nice to have" than a "must have" for any setting. It is absolutely a good idea for mod authors to add definitions to $settings_defs for their custom settings, and strongly recommended, but failure to do so will not cause custom settings to disappear during normal forum operation.
Slava Ukraini!
Heroiam slava!

I promise you nothing.

Sesqu... Sesqui... what?
Sesquipedalian, the best word in the English language.

danielrichter

Quote from: Arantor on December 31, 2021, 02:07:20 PMWhen you put in Sources/Cache/APIs/Redis.php, you could add a static method to that class just for the settings file hook, maybe let's call it integrate_update_settings_file as a method, so SMF\Cache\APIs\Redis::integrate_update_settings_file would be a static callable that does the relevant array manipulation needed, and then you would be able to put that as the function in the add_integration_function call (as a fully qualified callable name).

Dear @Arantor I've already noticed the whole /Sources/Cache/APIs/ dir is autoloaded, but how make sure the value is updated and how to use the callback (meaning how to trigger it now correctly)?
I don't get it, here's my Redis.php (autoloaded), required methods are implemented just because these are required, will "fill" them with redis calls after settings are stored:

<?php

namespace SMF\Cache\APIs;

use 
SMF\Cache\CacheApi;
use 
SMF\Cache\CacheApiInterface;

if (!
defined('SMF'))
die('No direct access...');

class 
Redis extends CacheApi implements CacheApiInterface
{
public static function integrate_update_settings_file ()
{
            
//how to make sure settings are stored?
            
$settings_defs["cache_redis"] = "sample_connection_string";
}

public function connect()
{
return true;
}

public function getData($key$ttl null)
{
return true;
}

public function putData($key$value$ttl null)
{
return true;
}

public function cleanCache($type '')
{
return true;
}

}

?>

Trigger install file:

<?php

//just for debug
//die(var_dump(SMF\Cache\APIs\Redis::integrate_update_settings_file()));

add_integration_function('integrate_update_settings_file''SMF\Cache\APIs\Redis::integrate_update_settings_file'true);


Arantor

Well, assuming you have run the trigger install (you can verify this by looking in smf_settings table under integrate_update_settings_file), all you should need to do is go to Admin > Maintenance > Cache Settings (I think that's where it is, it's not there on my copy but that's because I may have moved it) and try saving the settings.

That's really what the hook is about: adding it to the list of things that will be saved when the settings file is saved.

I think you'll also need to actually pass in $settings_defs as a parameter in your function, with a & to indicate that you're accepting it in a way it can be modified.
No good deed goes unpunished / All helpful urges should be circumvented

I have something to say: it's better to burn out than to fade away. There can be only one.

danielrichter

@Arantor thanks, already included an external redis library to talk to the redis server.

How to make use of the cache_redis variable?
Meaning how to access it, just using $cache_redis won't work I think?
It seems to be set, database:

integrate_update_settings_file : youraction_add_hook, SMF\Cache\APIs\Redis::integrate_update_settings_file, SMF\Cache\APIs\Redis::integrate_update_settings_files,SMF\Cache\APIs\Redis::integrate_update_settings_file
included settings_defs to manipulate existing settings:

    public static function integrate_update_settings_file (&$settings_defs)
    {
            $settings_defs["cache_redis"] = "blaaas";
    }

but how to "access" the variable now inside my "redis" code?

danielrichter

Any idea how I can access the "global" settings variable declared above?

Arantor

No good deed goes unpunished / All helpful urges should be circumvented

I have something to say: it's better to burn out than to fade away. There can be only one.

danielrichter

Thank you @Arantor
Think I will be able to release my redis cache mod the following weeks, so every redis fan can use it.

Is there an easy way how I can "store" something inside redis cache / trigger SMF to cache something inside?
Sessions are stored inside redis too?
Already tested the "Empty SMF's cache" function, working fine.

Arantor

Make sure it's configured, turn on caching level 1... as for getting something into cache... off the top of my head I think you'd want to go to the theme settings and turn on the 'recent posts on the board index' setting to at least 5 posts. That then should be cached in Redis for a few minutes at a time.
No good deed goes unpunished / All helpful urges should be circumvented

I have something to say: it's better to burn out than to fade away. There can be only one.

danielrichter

@Arantor

Yes I can see the code is triggered already:

<?php

namespace SMF\Cache\APIs;

require 
'predis/autoload.php';
\
Predis\Autoloader::register();

use 
SMF\Cache\CacheApi;
use 
SMF\Cache\CacheApiInterface;

if (!
defined('SMF'))
die('No direct access...');

class 
Redis extends CacheApi implements CacheApiInterface
{

public static function integrate_update_settings_file (&$settings_defs)
{
    
$settings_defs["cache_redis"] = "blaaas";
}

public function cleanCache($type '')
{
global $cache_redis;

$redis = new \Predis\Client();

$testRedisValue = (is_null($cache_redis)) ? "novalue" $cache_redis;

$redis->set('foo'$testRedisValue);

//$redis->flushAll();

return true;
}


public function connect()
{
return true;
}

public function getData($key$ttl null)
{
return true;
}

public function putData($key$value$ttl null)
{
return true;
}

}

?>

Using CLI I can verify:

127.0.0.1:6379> GET foo
"novalue"

But how can I access the $cache_redis value? Is my naming incorrect, because it seems to be null / or is my way of calling it incorrect?

Sesquipedalian

Quote from: danielrichter on January 24, 2022, 02:08:16 PMBut how can I access the $cache_redis value? Is my naming incorrect, because it seems to be null / or is my way of calling it incorrect?

I think you may have misunderstood what the integrate_update_settings_file hook is for, @danielrichter. That integration hook is not how you set the actual value in the Settings.php file. Instead, it allows you to tell the updateSettingsFile function information about your setting, such as what type of value it is (boolean, integer, string, etc.), what its default value should be, and the text to use as a descriptive comment (if any).

So, assuming the value of your setting is supposed to be a string, your hooked function should look something like this:
public static function integrate_update_settings_file(&$settings_defs)
{
$settings_defs["cache_redis"] = array(
'text' => '/* descriptive comment */',
'default' => 'novalue',
'type' => 'string',
);
}
If you want to get fancy and use some logic to ensure that your setting is inserted into the file near the other cache related settings, you can just loop through $settings_defs and find the right place to insert yours into the array.

To allow the admin to actually set the value of your $cache_redis setting, you will need to add a separate cacheSettings method to your class, similar to the ones used in the FileBased, SQLite, Memcache, and Memcached classes.
Slava Ukraini!
Heroiam slava!

I promise you nothing.

Sesqu... Sesqui... what?
Sesquipedalian, the best word in the English language.

danielrichter

#13
@Sesquipedalian but how to access the value after it's set?
No need to get fancy, I would just insert the string of redis connection inside my integrate_update_settings_file function.

Just unsure how I can access the value from example the constructor inside my Redis class, so I can initialize the connection.

I edited the integrate_update_settings_file to:

public static function integrate_update_settings_file (&$settings_defs)
{

$settings_defs["cache_redis"] = array(
'text' => '/* redis_connection_string */',
'default' => '127.0.0.1:6379',
'type' => 'string',
);

}

How to make sure SMF reads this data again and how can I access the value from my f.e. constructor?

Sesquipedalian

Once the value has been saved to Settings.php, you just use global $cache_redis; to access it, exactly as you are already doing in your cleanCache() method.

Your problem right now, at least in the code you posted above, is that you currently don't appear to be doing anything to actually save the value of the setting to Settings.php in the first place. You are using your integrate_update_settings_file() method to describe what the setting should look like when saved, but you are never doing anything to actually save it. Because of that, you will always end up with null when you try to retrieve a value from $cache_redis.

In order to solve the problem, you need to do as I said in my previous post:
Quote from: Sesquipedalian on January 24, 2022, 06:38:01 PMTo allow the admin to actually set the value of your $cache_redis setting, you will need to add a separate cacheSettings method to your class, similar to the ones used in the FileBased, SQLite, Memcache, and Memcached classes.
I suggest that you just copy and paste the code of that method from Sources/Cache/APIs/FileBased.php into your own class, and then tweak it as necessary to match your own needs. In all likelihood, that tweaking will need to involve nothing more than replacing each occurrence of the string "cachedir" with "cache_redis". Then you will be able to enter the actual value for your setting using the admin panel in SMF, and it will save to Settings.php. (You will also need to add a couple of $txt strings in a language file somewhere in order to provide labels for your settings in the admin panel, but that's trivial and I doubt I need to explain that. :) )
Slava Ukraini!
Heroiam slava!

I promise you nothing.

Sesqu... Sesqui... what?
Sesquipedalian, the best word in the English language.

danielrichter

@Sesquipedalian Included the language strings (into Themes/default/languages/ManageSettings.english.php) and verified the rendered HTML element too:

<input type="text" name="cache_redis" id="cache_redis" value="" size="36">
looks good from my point of view, but when I try to save it says:

Unknown config_var 'cache_redis'
Can I somehow retrigger the integrate_update_settings_file()? Because it seems like this was somehow broken during development.

    public static function integrate_update_settings_file (&$settings_defs)
    {
        $settings_defs["cache_redis"] = array(
            'text' => '/* redis connection string */',
            'default' => 'bla',
            'type' => 'string',
        );
    }

Using the Integration Hooks admin page I can see:


Sesquipedalian

I couldn't say without seeing your complete code.
Slava Ukraini!
Heroiam slava!

I promise you nothing.

Sesqu... Sesqui... what?
Sesquipedalian, the best word in the English language.

danielrichter

@Sesquipedalian my code:

Sources/Cache/APIs/Redis.php

I reduced code:

<?php

namespace SMF\Cache\APIs;

require 
'predis/autoload.php';
\
Predis\Autoloader::register();

use 
SMF\Cache\CacheApi;
use 
SMF\Cache\CacheApiInterface;

if (!
defined('SMF'))
die('No direct access...');

class 
Redis extends CacheApi implements CacheApiInterface
{
//used to register new redis config settings
public static function integrate_update_settings_file (&$settings_defs)
{
$settings_defs["cache_redis"] = array(
'text' => '/* redis connection string */',
'default' => 'bla',
'type' => 'string',
);
}

private $client;

public function __construct()
{
global $cache_redis;

$this->client = new \Predis\Client(null, ['prefix' => 'smf:']);

parent::__construct();

}

public function cacheSettings(array &$config_vars)
{
global $context$txt;

$class_name $this->getImplementationClassKeyName();
$class_name_txt_key strtolower($class_name);

$config_vars[] = $txt['cache_'$class_name_txt_key .'_settings'];
$config_vars[] = array('cache_redis'$txt['cachedir'], 'file''text'36'cache_cache_redis');

if (!isset($context['settings_post_javascript']))
$context['settings_post_javascript'] = '';

$context['settings_post_javascript'] .= '
$("#cache_accelerator").change(function (e) {
var cache_type = e.currentTarget.value;
$("#cache_redis").prop("disabled", cache_type != "'
$class_name .'");
});'
;
}

public function cleanCache($type '')
{
            return 
$this->client->flushAll();
}

public function connect()
{
return true;
}

public function getData($key$ttl null)
{
return $this->client->get($key);
}

public function putData($key$value$ttl null)
{
return $this->client->set($key$value'EX'$ttl !== null $ttl $this->ttl);
}

}

?>

I'm a bit confused because of the listed 3 integration hooks on admin panel. How to make sure integrate_update_settings_file() is triggered again, think after it's set the redis cache should work fine without any further edits to it.

danielrichter

Bump, still looking for a way to store redis settings, cache itself seems to be working, but it's much smarter to store settings of redis cache in SMF, not hardcoding them.

Sesquipedalian

Quote from: danielrichter on January 25, 2022, 02:59:50 PMlooks good from my point of view, but when I try to save it says:

Unknown config_var 'cache_redis'

It looks like you have stumbled upon a bug. Specifically, the saveSettings() function currently uses a hard-coded list of names for settings to store in Settings.php, and rejects all others. That list should not be hard-coded, so we will have to change the relevant code.

Quote from: danielrichter on January 27, 2022, 03:16:21 AMI'm a bit confused because of the listed 3 integration hooks on admin panel. How to make sure integrate_update_settings_file() is triggered again, think after it's set the redis cache should work fine without any further edits to it.

Uninstalling the mod should remove the hooks (assuming your package_info.xml file is built correctly). But since you appear to have multiple copies of the hook listed, you might need to go back to the integration hooks admin page after the package is uninstalled and manually remove the duplicate entries. A red X icon should appear in the Remove column for the duplicate entries, if there are any.
Slava Ukraini!
Heroiam slava!

I promise you nothing.

Sesqu... Sesqui... what?
Sesquipedalian, the best word in the English language.

Advertisement: