xoops forums

irmtfan

Module Developer
Posted on: 2013/5/4 1:55
irmtfan
irmtfan (Show more)
Module Developer
Posts: 3419
Since: 2003/12/7
#11

Re: SmartClone 1.10 Beta 1 ready for testing

Good.
I am not completely familiar with namespace.
Also php 5.3+ is not a problem because we need a solution for module cloning (in module level not core) and new modules like newbb, xoopspoll, ... required php 5.3
but there is another issue that i think namecpace is useless for us.
I think we must add something like this in all module files to avoid name coflict in different clones:
namespace basename(basename(dirname(__FILE__)));

or better:
namespace MY_MODULE\basename(basename(dirname(__FILE__)));

but the above cannot be used. Am i miss something and am i wrong?

@zyspec:
Yes i know how SmartClone works.
I want to know if there is a way to be free of Any naming issue in module scope.
once we can do that the install and even upgrade would be as simple as renaming the modules/MODULE_DIRNAME folder.

For example we can start with your new version of XoopsPoll as a very needed module for cloning.

zyspec

Module Developer
Posted on: 2013/5/4 3:40
zyspec
zyspec (Show more)
Module Developer
Posts: 1072
Since: 2004/9/21
#12

Re: SmartClone 1.10 Beta 1 ready for testing

Agree, we need to find a better cloning solution. Unfortunately we run into a class naming issue unless we use 'eval' which isn't a very good solution due to security risks.

The issue is finding a way to dynamically define a class name. This is required because of the way we access classes using the XOOPS API. So the following code:

xoops_getmodulehandler('myclass''mymodule');


loads:

class MymoduleMyclass() { }


but we can't define MymoduleMyclass dynamically without performing some 'tricks' (like using 'eval').

Another issue is that the way the core currently works will require us to do some "unique" things to the ./sql/mysql.sql file to manipulate the table names. This could be done by leaving the table names out of xoops_version.php and create/drop the tables in the ./include/oninstall.php and ./include/onuninstall.php files respectively.

irmtfan

Module Developer
Posted on: 2013/5/4 4:10
irmtfan
irmtfan (Show more)
Module Developer
Posts: 3419
Since: 2003/12/7
#13

Re: SmartClone 1.10 Beta 1 ready for testing

Yes. the mysql.sql file is one obstacle but it could be solved easily by defining a createTable() function in XoopsPersistableObjectHandler class. same is for definitions.
The main issue is function/class naming conflict. I still cannot see any solution for that even i want to know if changes in core can solve this? IMO even in xoops 2.6.0 it cannot be solved?

Mamba

Moderator
Posted on: 2013/5/4 6:38
Mamba
Mamba (Show more)
Moderator
Posts: 10876
Since: 2004/4/23
#14

Re: SmartClone 1.10 Beta 1 ready for testing

Quote:
Agree, we need to find a better cloning solution.

If we want to be efficient/pragmatic in our development, then I am not sure that we need something very different than SmartClone at the moment.

As long as a module is developed following the XOOPS standards, it should be cloneable out of the box by SmartClone. And it takes only few seconds to clone a module. So time-wise we're not going to save anything. The only difference will be between:

- you copying the module folder and then renaming it
- you clicking on a "Clone" button and providing a new name.

If we want to be more user friendly, then we could do one of these two:

- clone a module from within, like Publisher is doing now
- provide a cloning option as part of Core, but this is not the direction we're going with 2.6.0, where we are actually extracting features into separate modules.

So before we spend "development time" on this, we need to ask ourselves: what kind of time/resource savings or improvements will we achieve, if we make this change? Is it significant enough to justify the spending time for it?
Support XOOPS => DONATE
Use 2.5.10 | Docs | Modules | Bugs

zyspec

Module Developer
Posted on: 2013/5/4 12:09
zyspec
zyspec (Show more)
Module Developer
Posts: 1072
Since: 2004/9/21
#15

Re: SmartClone 1.10 Beta 1 ready for testing

@irmtfan,
Agree, table renaming can be solved relatively easily - even without core improvements if we have to...

You are right, class renaming is the biggest obstacle, and we might be able to solve this with namespaces, but I haven't spent enough time walking through how we'd implement this without core support to see if it's actually possible.

@Mamba,
I agree that trying to improve the current methods (either SmartClone or the using 'eval') in their current incarnation would not be high on the list of things to work on. The 'eval' method is much cleaner than SmartClone but given the potential security risks of 'eval' I think we're better of recommending SmartClone - thus I've removed 'eval' from all the modules I've been working on.

I do believe rethinking how module cloning works with some very specific goals in mind could be useful. One of the items on the list when reconsidering cloning methods should be the stipulation that majority of the code should be "common" between clones. This makes upgrading all copies of the module easier for administrators and keeps the overall footprint of the installation smaller.

Mamba

Moderator
Posted on: 2013/5/4 20:38
Mamba
Mamba (Show more)
Moderator
Posts: 10876
Since: 2004/4/23
#16

Re: SmartClone 1.10 Beta 1 ready for testing

Quote:
One of the items on the list when reconsidering cloning methods should be the stipulation that majority of the code should be "common" between clones. This makes upgrading all copies of the module easier for administrators and keeps the overall footprint of the installation smaller.

That's exactly what the D3 modules from GIJoe are doing, with the "clone" part being in the "normal" directory, and the "common" part being in the "trust path" folder (in our case, xoops_lib folder).

So I guess, our ultimate goal would be then to rewrite all XOOPS Modules to be "D3-modules"

This could be a lot of work, but the side benefit of it is that it will make easier to make in the future a "multi-site" version of XOOPS, because all the different clones of XOOPS will use the same "XOOPS_TRUST_PATH", and thus the same D3 modules.
Support XOOPS => DONATE
Use 2.5.10 | Docs | Modules | Bugs

irmtfan

Module Developer
Posted on: 2013/5/5 4:18
irmtfan
irmtfan (Show more)
Module Developer
Posts: 3419
Since: 2003/12/7
#17

Re: SmartClone 1.10 Beta 1 ready for testing

Yes as you know i always thought D3 way is the best for cloning.
Actually i always thought why we need to copy everything include all classes in all clones?
I dont put much time on it but it seems it will have many benefits like multisite ambition as you mentioned.

@zyspec:
IMO the first step forward to standardize all modules is writing them how that they have the least hard-coded $dirname.
for example the helper class can free us from the handler/config/module calling.
instead of writing these everywhere in our modules:
$myHandler xoops_getModuleHandler($name$dirname)


we can define a helper.php class (introduced in 2.6.0)
see:
http://svn.code.sf.net/p/xoops/svn/Xo ... /userlog/class/helper.php

and then use this helper class with ease:
$Userlog Userlog::getInstance();
// some codes here ...
    
$totalLogs $Userlog->getHandler('log')->getCount($criteria);


I strongly recommend using helper class in all 2.5.6 modules as well.
Adding this helper is very very easy. you just need to copy/paste the class from publisher or userlog.( publisher is better because it just has needed codes.)
http://svn.code.sf.net/p/xoops/svn/Xo ... isher/class/publisher.php

and then we are free from calling handler/config/module things.

Also debugging will be easier.

It will initialize all things like handler once:
function initHandler($name)
    {
        
$this->addLog('INIT ' $name ' HANDLER');
        
$this->handler[$name '_handler'] = xoops_getModuleHandler($name$this->dirname);
    }


So you just have one xoops_getModuleHandler function in the whole code in your module.
then in getHandler:
function &getHandler($name)
    {
        if (!isset(
$this->handler[$name '_handler'])) {
            
$this->initHandler($name);
        }
        
$this->addLog("Getting handler '{$name}'");
        return 
$this->handler[$name '_handler'];
    }


Then your module is ready for 2.6.0 too. (Now userlog can be installed in 2.6.0 ALPHA but of course with many deprecated and few strict errors)

once 2.6.0 turn to a final status, you as the module developer have very little job to do. just extend your helper class like this:
class Userlog extends Xoops_Module_Helper_Abstract
{
// additional codes here
 
}


irmtfan

Module Developer
Posted on: 2013/5/5 8:36
irmtfan
irmtfan (Show more)
Module Developer
Posts: 3419
Since: 2003/12/7
#18

Re: SmartClone 1.10 Beta 1 ready for testing

Quote:

zyspec wrote:
The 'eval' method is much cleaner than SmartClone but given the potential security risks of 'eval' I think we're better of recommending SmartClone - thus I've removed 'eval' from all the modules I've been working on.

What security risks do you mean?
I agree that eval is better than SmartClone way (replacing all instances)
But regarding eval we should note:
- Eval should be used when there is no way. eg: here in the core 2.5.6 all Oninstall/Onupdate and OnUninstall are hard-coded.
so we can use eval like this:
in include/module.php for manipulating xoops_module_pre_install_{$dirname} function we can use this:
eval( ' function xoops_module_pre_install_'.$dirname.'( $module ) { return onpreinstall_base( $module , "'.$dirname.'" ) ; } ' ) ;

function 
onpreinstall_base$module $dirname )
{
    
// TABLES (loading mysql.sql and change all $dirname)
    // CLASSES 
    // FUNCTIONS 
}


What do you think?

===================================

In 2.6 we have better functionality.
In 2.6 all install/update/uninstall functions has been turned to a preload but we need more regarding clone issue.(So 2.6 is a step forward!!!)
In 2.6 still pre functions in Oninstall/Onupdate and OnUninstall events are hard-coded.
I think in 2.6 we should turn SmartClone into a plugin
so pre install should be turned to a preload function like this:
In XOOPS26/modules/system/class/module.php:
"onModulePreInstall" event must be triggered before module installation. (before sql file runs)
so around line 173, I suggest adding following right before the legacy (2.5.6 way) onInstall script:
// START irmtfan to add a pre install preload event trigger
 
XoopsPreload::getInstance()->triggerEvent('onModulePreInstall', array(&$module, &$this));
 
// END irmtfan to add a pre install preload event trigger
 
$install_script $module->getInfo('onInstall'); // irmtfan - remained for legacy modules

Then we just need a Clone module(now call it extension!!!) and plugin.

In Clone module (or you can name it extenstion) we will have this in Xoops26/modules/clone/preloads/core.php
class CloneCorePreload extends XoopsPreloadItem
{
    
// To do anything before install the module
    
static function eventOnModulePreInstall($args)
    {
        
$module $args[0];
        if (
$plugin Xoops_Module_Plugin::getPlugin($module->getVar('dirname'), 'clone'true)) {
            Clone::
getInstance()->createClone($module);
        }   
    }
}

I think in createClone function it should change all instances of dirname ??? (but it can be customized in any module plugin.)
any module want to be clonable should have a plugin like this:
in modules/MODULE/class/plugin/clone.php
class {$Moduledirname}ClonePlugin extends Xoops_Module_Plugin_Abstract implements ClonePluginInterface
{
    public function 
clonable()
    {
        return 
true;
    }
    public function 
customClone()
    {
        
$custom = array();
        
$i=0;
        
$custom[$i]["key"] = "BLABLA";
        
$custom[$i]["replace"] = "REPLACE_CONSTANT";
        
        return 
$custom;        
    }
}

Mamba

Moderator
Posted on: 2013/5/5 8:52
Mamba
Mamba (Show more)
Moderator
Posts: 10876
Since: 2004/4/23
#19

Re: SmartClone 1.10 Beta 1 ready for testing

Quote:
What security risks do you mean?

See this discussion

The main issue is that some hosts turn the "eval" off, and any script using "eval" won't work correctly anymore.
Support XOOPS => DONATE
Use 2.5.10 | Docs | Modules | Bugs

zyspec

Module Developer
Posted on: 2013/5/5 13:52
zyspec
zyspec (Show more)
Module Developer
Posts: 1072
Since: 2004/9/21
#20

Re: SmartClone 1.10 Beta 1 ready for testing

It's pretty easy to break the 'eval' code above without some very stringent testing on the allowed directory (folder) names. There are, and probably always will be, ways to break it unless very careful filtering is done on the variables being evaluated in the 'eval' statement - which is one of the reasons the PHP development team discourages usage of 'eval' in a production environment.

As an example - suppose I want to install a clone of a module to handle all of my links for a specific series of car parts. I want a module that will only handle my '1000' series of parts, another module to handle my '2000' series of parts, etc...

I cannot put my module in the '1000' directory and expect it to work because using '1000' as the beginning of a class name in PHP is invalid. You cannot have class names like '1000Link' or '1000LinkHandler' - PHP does not allow it. This CAN BE corrected (by prefixing the module name with some known value). The point is valid though that without careful consideration of the possible issues using 'eval' will cause you could break the code or worse (open the system to security risks) without very careful coding.