Anthony Borrow added a comment - 26/Feb/09 08:04 AM

Anthony Borrow added a comment - 26/Feb/09 08:04 AM Joao - I am just getting this now and will try to look at it later this week. Thanks for sharing the code. Am I correct that you are wanting this block added to CONTRIB? Peace - Anthony

Anthony Borrow added a comment - 04/Mar/09 07:48 AM

Anthony Borrow added a comment - 04/Mar/09 07:48 AM Joao - Thanks for contributing the estatisticas block: As I have started to review and test it, I have a few suggestions, comment, questions. 1) First, I would encourage renaming the block to statistics 2) In terms of coding, I would rename variables to English read-able since others will be looking at the code and it is written in English I would avoid strings with names like verestats, etc. 3) I did not see the language file /lang/en_utf8/block_estatisticas.php 4) You should not need to connect to the database if you are requiring the config.php - I would encourage you to look at how Moodle gets records (check out the get_record and get_records which are defined in /lib/dmllib.php.Learning to do so would decrease your dependence on mysql and allow your block to function with other databases as well. I would also suggest that you test the code with debugging set to show All reasonable PHP errors (not developer mode). Doing so will help to catch notices like: Catchable fatal error: Object of class admin_root could not be converted to string in /home/arborrow/Moodle/code/19stable/admin/pagelib.php on line 149 Warning: mysql_connect() [function.mysql-connect]: Access denied for user '19stable'@'localhost' (using password: YES) in /home/arborrow/Moodle/code/19stable/blocks/estatisticas/dados1.php on line 31 Warning: mysql_connect() [function.mysql-connect]: Access denied for user '19stable'@'localhost' (using password: YES) in /home/arborrow/Moodle/code/19stable/blocks/estatisticas/dados2.php on line 28 At this point, I do not find the block to be functional and think we need to work on making some improvements to make it more stable before adding it to CVS. The reason I am hesitating is that we may decide to change the way the block is implemented and do away with dados1,php, dados2.php, dados3.php (which should also probably be renamed to their English equivalents). Unfortunately CVS does not deal well with file renaming and moving things around so I would rather work on that here and then upload. Let me know if you have any questions or if there is anything I can do to help. Peace - Anthony

Anthony Borrow added a comment - 04/Mar/09 09:39 AM

Anthony Borrow added a comment - 04/Mar/09 09:39 AM I did notice that you had the following typo that was causing the mysql_connect error: Instead of: $myUser = $CFG->dbname; I think that should be: $myUser = $CFG->dbuser; Peace - Anthony

Anthony Borrow added a comment - 04/Mar/09 09:47 AM

Anthony Borrow added a comment - 04/Mar/09 09:47 AM In this screenshot, you have hard coded the output in Portuguese. I think all of these should be converted to language strings. Let me know if you need help with that. It is just a matter of using get_string and of course creating the strings and language files. Peace - Anthony

Anthony Borrow added a comment - 08/Mar/09 09:00 AM

Anthony Borrow added a comment - 08/Mar/09 09:00 AM Joao - For now, I am going to reject your CVS write request (http://moodle.org/mod/cvsadmin/edituser.php?action=edit&user=247) only because I am trying to get caught up on reviewing all of the requests. Once you are able to patch things up in your block based on my comments above and we add the code to CVS I can go back and approve it. Peace - Anthony

Anthony Borrow added a comment - 08/Mar/09 10:54 AM

Anthony Borrow added a comment - 08/Mar/09 10:54 AM Joao - I just noticed that your block uses get_config and set_config for various settings. It is preferred that blocks (especially non-core blocks) use the mdl_config_plugins table rather than mdl_config. Let me know if you have questions about that. Peace - Anthony