Forum Preview

01/27/11 04:40pm

Status: offline

Roccivic

Forum User

Moderator

Registered: 05/19/10

Posts: 136

Hi there,

I wrote a prototype for the version control and dependencies feature request and I guess that I'm looking some feedback about it.

QUICK SUMMARY:

Plugins will old-style install can only require a minimum version of geeklog.
Plugins will new-style install can require a version of geeklog and any other plugin.
Each plugin will be able to include dependency information in autoinstall.inc.
To be able to install a plugin ALL of it's dependencies must be installed and enabled. And dependencies must be resolved after each operation (install, enable, etc).
If the dependencies for a plugin cannot be resolved, an installed plugin will be disabled and an uninstalled one will not be allowed to be installed.
if, for example, PLUGIN_A requires PLUGIN_B, and PLUGIN_B requires PLUGIN_A, the dependency cannot be resolved. But then, if these two plugins need each other so much, maybe they should just be merged into PLUGIN_C...
The load order of the plugins is checked every time after the dependencies have been resolved. So that if PLUGIN_A requires PLUGIN_B, PLUGIN_B is loaded before PLUGIN_A.
And I also renamed the "plugin editor" to "plugin information center" and moved all of its functionality out in the plugin list. And I added a similar functionality for disabled plugins.
The "Quick Uninstall" option. It requires JS, but has a fallback.
Added some JavaScript that ensures that the tooltips are not getting clipped. No fallback there

The dependency information would go into an array in the "plugin_autoinstall_myplugin()" function and, of course, then the array with the requirements should be included into the array that will be returned by the function. All of the following are examples of valid valid requirements:

Text Formatted Code

        $requires = array(

                array( 'name' => 'captcha', 'version' => '1.0.0', 'operator' => '>'),

                array( 'name' => 'links', 'version' => '1.7.1', 'operator' => '!=') // forbid v1.7.1 of links

        );

        $requires = array(

                array( 'name' => 'geeklog', 'version' => '1.8.0'), // default operator is '>='

                array( 'name' => 'links', 'version' => '1.0.0')

        );

        $requires = array(

                array( 'name' => 'socnet', 'version' => '1.0.0', 'operator' => '>'),

                array( 'name' => 'socnet', 'version' => '2.0.0', 'operator' => '<='),

        );

All the functionality that I wanted to put in there, is there. Some stuff doesn't work properly (yet).

SCREENSHOT:
http://www.placella.com/gl/plugin_admin.png

DEMO SITE:
First of all, thanks Ben for hosting the prototype, it's super appreciated

URL: http://demo4roccivic.geeklog.fr/gl-hg-version-JStooltip-upload/public_html/admin/plugins.php
It's last week's 1.8.0hg, so the new theme is not there.
The login details for "Admin" user have not been changed.
Please don't mess it up

The rating plugin on the demo site is an example of what doesn't work. It requires geeklog 1.9.0 (nonsense), but this requirement is not checked until the plugin is up and running (too late, btw). And only then it is disabled and the dependency is reported.

Suggestions? Recommendations? Comments? Tips?

Take care,
Rouslan

01/27/11 06:35pm

Status: offline

jmucchiello

Forum User

Full Member

Registered: 08/29/05

Posts: 985

Suggestions:

We can probably get rid of the "search" tool. Is someone really going to install 51+ plugins? (It just takes up screen real estate.)
Are the X and Check icons links that enable/disable the plugin? Is there a tooltip for that: "Click to Enable" or "Click to Disable"?
The "new plugin" section should have a third dependencies value: "Unknown" for plugins that don't have the "requires" info in their autoinstall.php. Currently the tooltip just says "N/A".
Add a requirement type: "must load before" and "must load after" so that plugins can also handle load order dependencies on their own. See example below.

Imagine the socnet plugin has a function in its functions.inc file SOC_FooBar() and your plugin calls that function in its functions.inc outside of a function call. Your plugin must load after socnet. So in your requires section (or a separate array) you would have:

Text Formatted Code

$requires  = array(

  array( 'name' => 'socnet', 'version' => '1.0.0', 'operator' => '>=', 'load' => 'after'), // must load after socnet

  array( 'name' => 'captcha', 'version' => '2.0.0') // requires captcha  2 or better. Does not care where it loads in relation to captcha.

);

$requires = array(

  array( 'name' => '*', 'load' => 'before' ) // load before all other plugins

);

$load_dependency = array(

  array( 'name' => '*', 'load' => 'after') // load last

);

'load' can be 'before' or 'after'. If name is '*', load must exist and version/operator are ignored.

The good thing is you don't have to worry about cyclical dependencies since you are just validating if the plugin can be installed/enabled. The operator has to figure out how to get the plugins in the right order.

01/28/11 10:06am

Status: offline

Laugh

Site Admin

Admin

Registered: 09/27/05

Posts: 1475

It looks great Roccivic.

Here are a few suggestions:
- I agree with Joe, we might as well get rid of the search.
- Plugin Information Center. The button could say just "Close". Maybe for just the old Professional theme do not have the form center on the screen since no other admin form does.

I was thinking on Joe's "load after" idea and I thought of another scenario. Can a plugin author show say for example that the captcha plugin version 1.0.0 can be used but is not necessarily required (which is true for a number of plugins)? The Admin plugin form wouldn’t be required to enforce anything here but it is information that the Admin may want. I guess the only downfall here is if version 0.9 of the captcha is installed then the plugin itself would have to check to make sure that the correct version is installed before making any calls of the captcha plugin.

To account for this idea I guess you could do something like this:

Text Formatted Code

        $requires = array(

                array( 'name' => 'captcha', 'version' => '1.0.0', 'operator' => '>', ‘dependant’ => false),

                array( 'name' => 'links', 'version' => '1.7.1', 'operator' => '!=') // forbid v1.7.1 of links

        );

One of the Geeklog Core Developers.

01/28/11 07:38pm

Status: offline

Roccivic

Forum User

Moderator

Registered: 05/19/10

Posts: 136

Quote by: jmucchiello

Suggestions:
We can probably get rid of the "search" tool. Is someone really going to install 51+ plugins? (It just takes up screen real estate.)

Ok, I'll look into this.

Quote by: jmucchiello

Are the X and Check icons links that enable/disable the plugin? Is there a tooltip for that: "Click to Enable" or "Click to Disable"?

Yes, that's the function of those buttons. And no, no tooltips (yet). In my defence, though, there is a "legend" above the search box

Quote by: jmucchiello

The "new plugin" section should have a third dependencies value: "Unknown" for plugins that don't have the "requires" info in their autoinstall.php. Currently the tooltip just says "N/A".

Yeah, I put the N/A there, couldn't think of any better words at the time.

Quote by: jmucchiello

Add a requirement type: "must load before" and "must load after" so that plugins can also handle load order dependencies on their own. See example below.

Imagine the socnet plugin has a function in its functions.inc file SOC_FooBar() and your plugin calls that function in its functions.inc outside of a function call. Your plugin must load after socnet. So in your requires section (or a separate array) you would have:

Text Formatted Code

$requires  = array(

  array( 'name' => 'socnet', 'version' => '1.0.0', 'operator' => '>=', 'load' => 'after'), // must load after socnet

  array( 'name' => 'captcha', 'version' => '2.0.0') // requires captcha  2 or better. Does not care where it loads in relation to captcha.

);

$requires = array(

  array( 'name' => '*', 'load' => 'before' ) // load before all other plugins

);

$load_dependency = array(

  array( 'name' => '*', 'load' => 'after') // load last

);

Sorry, I don't really understand this. I mean, right now the load order is automagically rearranged to take dependencies into account. And the operator (user) is simply not allowed to move the plugins into a place in the load order where this will conflict with the order required by the dependencies. I can't see why this is wrong.

How about just allowing plugins to bypass the check? Right now all plugins are just given a load order of '10000' when installed, which is a very low priority. This value is then quickly readjusted for display purposes.

Text Formatted Code
$requires = array(

  // default: make sure that the parent plugin is loaded first only

  array('name' => 'myplugin1', 'version' => '1.0.0'),

  // don't care when to be loaded

  array('name' => 'myplugin1', 'version' => '1.0.0', 'loadorder' => 'ignore'),

);

Related: Would it be useful to cache the dependencies? So that we don't call the autoinstall.inc of "PluginThatRequiresSocnet" to resolve its dependencies (more than once), if "SocNet" is disabled.

Quote by: Laugh

It looks great Roccivic.

Thanks

Quote by: Laugh

- Plugin Information Center. The button could say just "Close". Maybe for just the old Professional theme do not have the form center on the screen since no other admin form does.

Ok.

Quote by: Laugh

I was thinking on Joe's "load after" idea and I thought of another scenario. Can a plugin author show say for example that the captcha plugin version 1.0.0 can be used but is not necessarily required (which is true for a number of plugins)? The Admin plugin form wouldn’t be required to enforce anything here but it is information that the Admin may want. I guess the only downfall here is if version 0.9 of the captcha is installed then the plugin itself would have to check to make sure that the correct version is installed before making any calls of the captcha plugin.

I think that as it is, it's already a big patch and I don't even know if it'll make it for 1.8.0. But, I guess that we could add more arrays at a later date in the autoinstall.inc, like:

Text Formatted Code

// Self explanatory?

$recommends = array(...);

$suggests = array(...);

$replaces = array(...);

$provides = array(...);

It would be neater and completely expandable. What do you think?

@Laugh: Also, what did you make of the JS function on the demo site that kept the tooltips from disappearing off the screen? The code maybe should be cleaned up a bit, but the idea I think is good. Do you want a copy of the cleaned-up version? Or were you going to look into a CSS-only solution to that?

Other then these issues, anything else? Or should I go ahead and write the serious implementation?

Oh and I also have a question. I want to break all this work into several smaller patches, possibly 5 or 6. What is the best way for me to proceed now? Take a fork of the repo and code there without pulling, submit the patches and hope for the best? I'm kind of afraid of pulling and executing "hg update" when I already have made modifications. Last time I tried, hg forked an instance of meld (http://en.wikipedia.org/wiki/Meld_%28software%29) with a three way diff and I had no idea what it meant... Is there a trick or am I just missing something?

Thanks all for the feedback

Rouslan.

01/29/11 10:09am

Status: offline

Laugh

Site Admin

Admin

Registered: 09/27/05

Posts: 1475

@Laugh: Also, what did you make of the JS function on the demo site that kept the tooltips from disappearing off the screen? The code maybe should be cleaned up a bit, but the idea I think is good. Do you want a copy of the cleaned-up version? Or were you going to look into a CSS-only solution to that?

Email it to me so I can see if the solution works with the new theme.

Other then these issues, anything else? Or should I go ahead and write the serious implementation?

I would go ahead and write it.

Oh and I also have a question. I want to break all this work into several smaller patches, possibly 5 or 6. What is the best way for me to proceed now? Take a fork of the repo and code there without pulling, submit the patches and hope for the best? I'm kind of afraid of pulling and executing "hg update" when I already have made modifications. Last time I tried, hg forked an instance of meld (http://en.wikipedia.org/wiki/Meld_%28software%29) with a three way diff and I had no idea what it meant... Is there a trick or am I just missing something?

The way I would handle this is to clone the main repository, do the work and when you are finished, then merge any changes from the main repository. Once that is complete you can then commit your work to your repository and create just 1 patch. I am not sure why you want to make 5 or 6 patches. I am not an expert with Mercurial though so send Dirk an email to explain what you want to do. I actually use TortoiseHG to give Mercurial a graphical interface to make things easier (at least for me).

Tom

One of the Geeklog Core Developers.

01/30/11 05:54pm

Status: offline

Roccivic

Forum User

Moderator

Registered: 05/19/10

Posts: 136

So, I think that I'm pretty much finished with this now.
I put up a copy up online here: http://demo4roccivic.geeklog.fr/geeklog/public_html/admin/plugins.php
It's yesterday's 1.8.0hg with my modifications. Please let me know what you make of it.

I've removed the search bar and the legend that I had in the prototype and I added more tooltips. Title tags on images that is, not COM_tooltip(). But I have not added

As far as the code goes, I made 15 commits that fix 2 feature requests. After pulling and updating I ended up with a fork (I think) and I'm not too sure what is the best way to submit this for review. So, do I export 15 patches each as a file and zip them together or should I create just one big patch? Actually I'm using TortoiseHG, too now. Any tips are very welcome.

Here's a screenshot of what my repository looks like now:
http://www.placella.com/gl/screenshot.png

Rouslan

01/30/11 08:01pm

Status: offline

Laugh

Site Admin

Admin

Registered: 09/27/05

Posts: 1475

Hmm, the only issue I see at the moment is if the plugin is disabled the image for it doesn't get returned on the plugin information screen.

You should be able to submit your work as one patch. Click on the head and select the merge with option. It will then merge the fork you currently are working on with the HEAD and then you can submit that commit as a patch.

Forks happen when you commit work and then pull changesets. If you do not want to create a fork always pull the changes first (this should automatically merge them) before committing your work. I some times miss doing this and create a fork when I do not want too.

Also you can always backup your work directory (by just making a copy of it) just incase something gets messed up with the merge or commit.

Tom

One of the Geeklog Core Developers.

01/31/11 10:47am

Status: offline

Roccivic

Forum User

Moderator

Registered: 05/19/10

Posts: 136

Quote by: Laugh

Hmm, the only issue I see at the moment is if the plugin is disabled the image for it doesn't get returned on the plugin information screen.

I've traced this problem to lib-plugins.php, in particular this snippet of code from PLG_getIcon():

Text Formatted Code

    // lastly, search for the icon (assuming it's a GIF)

    if (empty($retval)) {

        $icon = $_CONF['site_url'] . '/' . $type . '/images/' . $type . '.gif';

        $fh = @fopen ($icon, 'r');

        if ($fh === false) {

            $icon = $_CONF['site_admin_url'] . '/plugins/' . $type . '/images/'

                  . $type . '.gif';

            $fh = @fopen ($icon, 'r');

            if ($fh === false) {

                // give up and use a generic icon

                $retval = $_CONF['site_url'] . '/images/icons/plugins.gif';

            } else {

                $retval = $icon;

                fclose ($fh);

            }

        } else {

            $retval = $icon;

            fclose ($fh);

        }

    }

    return $retval;

}

First of all, why doesn't this handle PNG images? Oh, well...
But the real problem here is that if you try to "fopen($url, $mode)", it will return true even if the full URL does not exist (but the server does). In fact if you would proceed with fread(), you'd get a nice 404 page, too. I think that the correct way to handle this is too look for a path (e.g: "/home/mysite/foo.png" ) instead and then return the corresponding url. I've uploaded the fix to the demo site and it seems to work fine. Here it is (one to one replacement for the above code):

Text Formatted Code
    // lastly, search for the icon

    if (empty($retval)) {

        $icons[] = $type . '/images/' . $type . '.png';

        $icons[] = $type . '/images/' . $type . '.gif';

        $icons[] = 'admin/plugins/' . $type . '/images/' . $type . '.png';

        $icons[] = 'admin/plugins/' . $type . '/images/' . $type . '.gif';

        foreach ($icons as $key => $value) {

            if (is_readable($_CONF['path_html'] . '/' . $value)) { // look for a path: e.g.: "/home/foo/bar.png"

                $retval = $_CONF['site_url'] . '/' . $value; // but return a url: e.g.: "http://foo.com/bar.png"

                break;

            }

        }

    }

    // Still nothing? Give up and use a generic icon

    if (empty($retval)) {

        $retval = $_CONF['site_url'] . '/images/icons/plugins.gif';

    }

    return $retval;

I guess that I'll file a separate bug for this.
Also I wonder if the only input variable to the PLG_getIcon(string $type) function should be sanitised for file access. Anyone?

Quote by: Laugh

You should be able to submit your work as one patch. Click on the head and select the merge with option. It will then merge the fork you currently are working on with the HEAD and then you can submit that commit as a patch.

Thanks, I'll try that.

Rouslan