Re: [ppa-dev] New PPA architecture plugin

[Leonardo S. <l.s...@gm...>]

Thanks Jehan, below the answers:


>  * ./classes/plugins/PluginManager.php should be in
> ./classes/PluginManager.php

Ok, I moved it and dropped the plugins/ directory

>  * this comment is malformed :
>
>      // Plugins
>      // Add index to plugins that will be activated.
>      // Example: array('PluginExample', 'Slony', ...)
>      $conf['plugins'] = array();
>
>    I would write:
>
>      /** Plugins management
>      * Add plugin names to the following array to activate them
>      * Example: $conf['plugins'] = array('PluginExample', 'Slony');
>      */

Ok, changed


>  * we shouldn't require a Makefile in plugins/{$plugin_name}/lang/. Instead,
> we should patch lang/Makefile and other needed files to take care of plugins
> when executing "make all" or "make lang_name"

How can I do it? :)


>  * add a simple page to your plugin pluginExample where the links it adds to
> the interface point to. This page might just have a link to go back to PPA's
> server page.

Done


>  * rise a human understable error if a plugin given in configuration doesn't
> exists. Make sure to specify this is case sensitive.

Done


>  * couldn't we get rid of the "plugins/{$plugin_name}/lang/translations.php"
> file ?

Dropped


>  * why do we call a method from the plugin_manager to load translation ?
> couldn't we just check if the translation file exists and load it directly
> from the plugin class ?

I think all plugins will need to deal with translations, that's why I
put it in the PluginManager. Can be that?


>  * plugin should register themselves in the plugin_manager instead of doing
> it from lib.inc.php. ie. call "$plugin_manager->add_plugin($plugin)" from
> the plugin constructor.

Done, I also move a bunch of code from lib.inc.php to the
PluginManager constructor. I think it would be better to stay there,
but anyway, suggestions are welcome.


>  * I guess plugin can register their callback functions passing an array of
> function names in add_plugin as well.

Done


>  * IMO, attributes in plugins class don't need to be prefixed by 'plugin_'.
> Moreover, I would rename $this->plugin_name as $this->desc and
> $this->plugin_index as $this->name.

Ok, but I will not use more the $desc, because I realized that it will
have translation values, so I move it to lang files.


>  * don't use __CLASS__ but an explicit name. Here, PluginExample should be
> 'example'. It could be usefull and more human if we want to add a list of
> enabled plugins on the introduction page.

I changed to Example. Can be that?


> I might have some more comments in future, but let's discuss those first.

Sure :)

Jehan and Andreas, the link of all these commits is
[http://bit.ly/lRzW25]. Do you want a patch (all_in_one) with these
changes.

--
Atenciosamente
Leonardo Augusto Sápiras
[http://www.leonardosapiras.com.br]



2011/5/18 Jehan-Guillaume (ioguix) de Rorthais <io...@fr...>:
> On 11/05/2011 18:36, Leonardo Sápiras wrote:
>>
>>   Hi devs,
>>
>>   Following my GSoC activities
>>
>> [http://wiki.postgresql.org/wiki/New_phpPgAdmin_Plugin_Architecture_GSoC_2011],
>> Below some items that I have worked:
>>
>> [...]
>>
>>   Attached a patch with this. I also pushed it to a new branch in my
>> ppa fork, the GitHub link is [http://bit.ly/jGFbD2].
>>
>>   Jehan and Andreas, could you take a look at this?
>
> Here are my comments:
>
>  * ./classes/plugins/PluginManager.php should be in
> ./classes/PluginManager.php
>
>  * this comment is malformed :
>
>      // Plugins
>      // Add index to plugins that will be activated.
>      // Example: array('PluginExample', 'Slony', ...)
>      $conf['plugins'] = array();
>
>    I would write:
>
>      /** Plugins management
>      * Add plugin names to the following array to activate them
>      * Example: $conf['plugins'] = array('PluginExample', 'Slony');
>      */
>
>  * we shouldn't require a Makefile in plugins/{$plugin_name}/lang/. Instead,
> we should patch lang/Makefile and other needed files to take care of plugins
> when executing "make all" or "make lang_name"
>
>  * add a simple page to your plugin pluginExample where the links it adds to
> the interface point to. This page might just have a link to go back to PPA's
> server page.
>
>  * rise a human understable error if a plugin given in configuration doesn't
> exists. Make sure to specify this is case sensitive.
>
>  * couldn't we get rid of the "plugins/{$plugin_name}/lang/translations.php"
> file ?
>
>  * why do we call a method from the plugin_manager to load translation ?
> couldn't we just check if the translation file exists and load it directly
> from the plugin class ?
>
>  * plugin should register themselves in the plugin_manager instead of doing
> it from lib.inc.php. ie. call "$plugin_manager->add_plugin($plugin)" from
> the plugin constructor.
>
>  * I guess plugin can register their callback functions passing an array of
> function names in add_plugin as well.
>
>  * IMO, attributes in plugins class don't need to be prefixed by 'plugin_'.
> Moreover, I would rename $this->plugin_name as $this->desc and
> $this->plugin_index as $this->name.
>
>  * don't use __CLASS__ but an explicit name. Here, PluginExample should be
> 'example'. It could be usefull and more human if we want to add a list of
> enabled plugins on the introduction page.
>
> I might have some more comments in future, but let's discuss those first.
>
> Thx!
>
>>   Cheers!
>>
>> --
>> Atenciosamente
>> Leonardo Augusto Sápiras
>> [http://www.leonardosapiras.com.br]
>>
>>
>> ------------------------------------------------------------------------------
>> Achieve unprecedented app performance and reliability
>> What every C/C++ and Fortran developer should know.
>> Learn how Intel has extended the reach of its next-generation tools
>> to help boost performance applications - inlcuding clusters.
>> http://p.sf.net/sfu/intel-dev2devmay
>> _______________________________________________
>> phpPgAdmin-devel mailing list
>> php...@li...
>> https://lists.sourceforge.net/lists/listinfo/phppgadmin-devel
>
>