Non-core contributed modules

META: Make improvements to NEWMODULE.zip

Details

  • Type: Improvement Improvement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.9.4
  • Fix Version/s: None
  • Component/s: Module: Newmodule
  • Labels:
    None
  • Affected Branches:
    MOODLE_19_STABLE

Description

Meta bug for improvements which need to be made to newmodule template

Issue Links

This issue is duplicated by:
CONTRIB-52 Improvements to make NEWMODULE really useful Minor Closed
This issue has a non-specific relationship to:
CONTRIB-1385 META: NEWMODULE 2.0 Minor Resolved

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

I've closed CONTRIB-52 (the original NEWMODULE contrib bug) in benefit of this. Ciao

Show
Eloy Lafuente (stronk7) added a comment - I've closed CONTRIB-52 (the original NEWMODULE contrib bug) in benefit of this. Ciao
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Also, some days ago I put 19_STABLE and HEAD in sync, copying some missing bits here and there and cleaning whitespace to have both ready for any improvement. I think those are the branches we should try to improve.

Show
Eloy Lafuente (stronk7) added a comment - Also, some days ago I put 19_STABLE and HEAD in sync, copying some missing bits here and there and cleaning whitespace to have both ready for any improvement. I think those are the branches we should try to improve.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Finally, here there are some notes I had in my TODO about NEWMODULE. Perhaps it would be a good idea to convert them into subtasks:

  • NEWMODULE: Add official credits
  • NEWMODULE: Add savepoints on upgrade
  • NEWMODULE: Add capabilities
  • NEWMODULE: Add users + files + grades
  • NEWMODULE: Add backup & restore
  • NEWMODULE: Add 2.0 stuff (files, DB, install/upgrade...)
Show
Eloy Lafuente (stronk7) added a comment - Finally, here there are some notes I had in my TODO about NEWMODULE. Perhaps it would be a good idea to convert them into subtasks:
  • NEWMODULE: Add official credits
  • NEWMODULE: Add savepoints on upgrade
  • NEWMODULE: Add capabilities
  • NEWMODULE: Add users + files + grades
  • NEWMODULE: Add backup & restore
  • NEWMODULE: Add 2.0 stuff (files, DB, install/upgrade...)
Hide
Permalink
Daniele Cordella added a comment -
  • NEWMODULE: Add management of groups
Show
Daniele Cordella added a comment -
  • NEWMODULE: Add management of groups
Hide
Permalink
David Mudrak added a comment -

I just merged my recent fixes from 1.9 to HEAD. They include:

  • use nested dirname() instead of "../../" in require_once() paths
  • use get_coursemodule_from_id() instead of plain get_record() [security issue]
  • require_login() - provide the whole $course object if available [performance]
  • do not indent the main program level of source code
    + some small details
Show
David Mudrak added a comment - I just merged my recent fixes from 1.9 to HEAD. They include:
  • use nested dirname() instead of "../../" in require_once() paths
  • use get_coursemodule_from_id() instead of plain get_record() [security issue]
  • require_login() - provide the whole $course object if available [performance]
  • do not indent the main program level of source code + some small details
Hide
Permalink
Chris Barrett added a comment -

Not sure if this is the correct place to put this, but according to netbeans, on line 52 of view.php in newmodule, there is a syntax error.
$strwidget = get_string('modulename', "widget');

The double quote in front of widget should be a single quote.
Thanks!
Chris

Show
Chris Barrett added a comment - Not sure if this is the correct place to put this, but according to netbeans, on line 52 of view.php in newmodule, there is a syntax error. $strwidget = get_string('modulename', "widget'); The double quote in front of widget should be a single quote. Thanks! Chris
Hide
Permalink
Anthony Borrow added a comment -

Chris - Yep, this looked like something that slipped past David. I've gone ahead and committed the fix. Thanks for reporting it. Peace - Anthony

Show
Anthony Borrow added a comment - Chris - Yep, this looked like something that slipped past David. I've gone ahead and committed the fix. Thanks for reporting it. Peace - Anthony
Hide
Permalink
David Mudrak added a comment -

My mistake during the merge - sorry and thanks!

Show
David Mudrak added a comment - My mistake during the merge - sorry and thanks!
Hide
Permalink
Daniele Cordella added a comment -

Ciao moodlers.
Yesterday, playing with reports, I arrived to the "Activity report" and I got an error message pointing to the returned value of the function NEWMODULE/lib.php::function newmodule_user_outline

Actually I found that, in the NEWNMODULE layout provided in http://download.moodle.org/plugins/mod/NEWMODULE.zip,
the declared function (row 98 of lib.php) closes with:
return $return;
and the variable $return is not defined... fine!!!
What is getting me crazy is why I can't reproduce the bug any more.

Hoping this can help you, David, to drop off this silly bug.

Show
Daniele Cordella added a comment - Ciao moodlers. Yesterday, playing with reports, I arrived to the "Activity report" and I got an error message pointing to the returned value of the function NEWMODULE/lib.php::function newmodule_user_outline Actually I found that, in the NEWNMODULE layout provided in http://download.moodle.org/plugins/mod/NEWMODULE.zip, the declared function (row 98 of lib.php) closes with: return $return; and the variable $return is not defined... fine!!! What is getting me crazy is why I can't reproduce the bug any more. Hoping this can help you, David, to drop off this silly bug.
Hide
Permalink
David Mudrak added a comment -

Daniele,

thanks for the spot. I have added the variable initialization.

Show
David Mudrak added a comment - Daniele, thanks for the spot. I have added the variable initialization.
Hide
Permalink
Daniele Cordella added a comment -

Ciao David,
it seems that the function

/**

  • Tells if files in moddata are trusted and can be served without XSS protection.
  • @return bool true if file can be served to all users (trusted), false otherwise
    */
    function newmodule_is_moddata_trusted() {
    return false;
    }

is missing from newmodule/lib.php.

Show
Daniele Cordella added a comment - Ciao David, it seems that the function /**
  • Tells if files in moddata are trusted and can be served without XSS protection.
  • @return bool true if file can be served to all users (trusted), false otherwise */ function newmodule_is_moddata_trusted() { return false; }
is missing from newmodule/lib.php.
Hide
Permalink
David Mudrak added a comment -

Hey Daniele,

the function is not in the template. However, it is not required by the core. The file.php checks if this function exists. IMO we should try to keep NEWMODULE as simple as possible and not to overload beginners with a lot of interface functions. So my proposal is to stay with the required module API only. If a developer wants her module to play with files and content security nicely, she should read file.php anyway so she will spot the existence of the function (like you did I suppose )

I am going to ask Martin D and others at HQ for their opinion

Show
David Mudrak added a comment - Hey Daniele, the function is not in the template. However, it is not required by the core. The file.php checks if this function exists. IMO we should try to keep NEWMODULE as simple as possible and not to overload beginners with a lot of interface functions. So my proposal is to stay with the required module API only. If a developer wants her module to play with files and content security nicely, she should read file.php anyway so she will spot the existence of the function (like you did I suppose ) I am going to ask Martin D and others at HQ for their opinion
Hide
Permalink
Daniele Cordella added a comment -

Thanks David for your reply.
I hope to find the time to share here my "empty" model/layout of newmodule before your chat with HQ.
IMO it is a good starting point for every newmodule and I use it with success.
Ciao.

Show
Daniele Cordella added a comment - Thanks David for your reply. I hope to find the time to share here my "empty" model/layout of newmodule before your chat with HQ. IMO it is a good starting point for every newmodule and I use it with success. Ciao.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Hi,

first of all, thanks for any effort improving NEWMODULE!

then, one silly question: Are you keeping both 19_STABLE and HEAD improved or only HEAD ?

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Hi, first of all, thanks for any effort improving NEWMODULE! then, one silly question: Are you keeping both 19_STABLE and HEAD improved or only HEAD ? Ciao
Hide
Permalink
David Mudrak added a comment -

This is the result of the HQ chat: all optional functions will be at the end of lib.php after a clear comment line

/**

  • Following stuff are OPTIONAL functions that may be enabled depending of your module needs
    */

Eloy: so far, we are trying to maintain both 19 and HEAD. Although I am focusing on HEAD mainly.

Show
David Mudrak added a comment - This is the result of the HQ chat: all optional functions will be at the end of lib.php after a clear comment line /**
  • Following stuff are OPTIONAL functions that may be enabled depending of your module needs */
Eloy: so far, we are trying to maintain both 19 and HEAD. Although I am focusing on HEAD mainly.
Hide
Permalink
Daniele Cordella added a comment -

> Eloy: so far, we are trying to maintain both 19 and HEAD. Although I am focusing on HEAD mainly.

Yes, this seems to be a problem. I just downloaded the last NEWMODULE.zip from
http://download.moodle.org/plugins/mod/NEWMODULE.zip
but I found:
$this->add_intro_editor(false, get_string('introduction', 'newmodule'));
in mod_form.php
but this function is still not defined in Moodle 1.9

We should provide two different links to download NEWMODULE.zip maybe in this thread.
Where is the NEWMODULE.zip for Moodle 1.9 now?

[Gosh!!! I am starting to write "We should provide..." instead of "It should be provided..."... is this dangerous? :-)]

Show
Daniele Cordella added a comment - > Eloy: so far, we are trying to maintain both 19 and HEAD. Although I am focusing on HEAD mainly. Yes, this seems to be a problem. I just downloaded the last NEWMODULE.zip from http://download.moodle.org/plugins/mod/NEWMODULE.zip but I found: $this->add_intro_editor(false, get_string('introduction', 'newmodule')); in mod_form.php but this function is still not defined in Moodle 1.9 We should provide two different links to download NEWMODULE.zip maybe in this thread. Where is the NEWMODULE.zip for Moodle 1.9 now? [Gosh!!! I am starting to write "We should provide..." instead of "It should be provided..."... is this dangerous? :-)]
Hide
Permalink
Daniele Cordella added a comment - - edited

ok. This is the "empty layout" I use for my personal use.
Let me fire a suggestion:
1. please, try and review it
2. when it is optimized, I offer my contribution to add to this "empty layout":
-> a simple mform to post data
-> a simple flexible_table with posted data
-> a simple capability to distinguish between users
-> the creation of a PDF with posted data
-> logs

eventually:
-> score
-> backup
-> restore

never:
-> groups

Please take care because I used NEWMODULE and newmodule.
If you want to test my newmodule.zip, take care to be case sensitive at the find and replace time.
I ONLY WORK FOR MOODLE 1.9
I KNOW NOTHING ABOUT HEAD

Please let me know.
Ciao.

Show
Daniele Cordella added a comment - - edited ok. This is the "empty layout" I use for my personal use. Let me fire a suggestion: 1. please, try and review it 2. when it is optimized, I offer my contribution to add to this "empty layout": -> a simple mform to post data -> a simple flexible_table with posted data -> a simple capability to distinguish between users -> the creation of a PDF with posted data -> logs eventually: -> score -> backup -> restore never: -> groups Please take care because I used NEWMODULE and newmodule. If you want to test my newmodule.zip, take care to be case sensitive at the find and replace time. I ONLY WORK FOR MOODLE 1.9 I KNOW NOTHING ABOUT HEAD Please let me know. Ciao.
Hide
Permalink
David Mudrak added a comment -

I decided to create CONTRIB-1385 to track modifications of NEWMODULE needed for Moodle 2.0. This issue will remain here to track improvements done at 1.9 branch.

Show
David Mudrak added a comment - I decided to create CONTRIB-1385 to track modifications of NEWMODULE needed for Moodle 2.0. This issue will remain here to track improvements done at 1.9 branch.
Hide
Permalink
Daniele Cordella added a comment -

Ciao David and congratulation for http://moodle.org/blog/index.php?postid=5430

After a discussion with Petr I found... in the just downloaded NEWMODULE package, in the file index.php
foreach ($newmodules as $newmodule) {
if (!$newmodule->visible) { //Show dimmed if the mod is hidden $link = "<a class=\"dimmed\" href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>"; } else { //Show normal if the mod is visible $link = "<a href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>"; }

where:
$link = "<a class=\"dimmed\" href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>"
should be:
$link = "<a class=\"dimmed\" href=\"view.php?id=$newmodule->coursemodule\">format_string($newmodule->name)</a>"

and
$link = "<a href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>";
should be:
$link = "<a href=\"view.php?id=$newmodule->coursemodule\">format_string($newmodule->name)</a>";

otherwise multilang will be breaked.
Do you agree?

Show
Daniele Cordella added a comment - Ciao David and congratulation for http://moodle.org/blog/index.php?postid=5430 After a discussion with Petr I found... in the just downloaded NEWMODULE package, in the file index.php foreach ($newmodules as $newmodule) { if (!$newmodule->visible) { //Show dimmed if the mod is hidden $link = "<a class=\"dimmed\" href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>"; } else { //Show normal if the mod is visible $link = "<a href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>"; } where: $link = "<a class=\"dimmed\" href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>" should be: $link = "<a class=\"dimmed\" href=\"view.php?id=$newmodule->coursemodule\">format_string($newmodule->name)</a>" and $link = "<a href=\"view.php?id=$newmodule->coursemodule\">$newmodule->name</a>"; should be: $link = "<a href=\"view.php?id=$newmodule->coursemodule\">format_string($newmodule->name)</a>"; otherwise multilang will be breaked. Do you agree?
Hide
Permalink
David Mudrak added a comment -

Ciao Daniele, and thanks for spotting this. I just committed your fix into CVS.

Show
David Mudrak added a comment - Ciao Daniele, and thanks for spotting this. I just committed your fix into CVS.
Hide
Permalink
Daniele Cordella added a comment -

Thanks David.
I also found that the set of three functions: newmodule_reset_course_form_definition, workshop_reset_course_form_defaults and workshop_reset_userdata described below, are still missing.
/**

  • Called by course/reset.php
  • @param $mform form passed by reference
    */
    function newmodule_reset_course_form_definition(&$mform) {

$mform->addElement('header', ' newmoduleheader', get_string('modulenameplural', 'newmodule'));
$mform->addElement('checkbox', 'reset_newmodule_all', get_string('resetnewmoduleall','newmodule'));
}

/**

  • Course reset form defaults.
    */
    function workshop_reset_course_form_defaults($course) {
    return array('reset_newmodule_all'=>1);
    }

/**

  • This function is used by the reset_course_userdata function in moodlelib.
  • This function will remove all issued certificates from the specified course
  • @param $data the data submitted from the reset course.
  • @return array status array
    */
    function workshop_reset_userdata($data) {
    global $CFG;

$componentstr = get_string('modulenameplural', 'newmodule');
$typesstr = get_string('resetcertificateall', 'newmodule');
$status = array()
if (!empty($data->reset_newmodule_all)) { // purge tables and moodledata if needed $status[] = array('component'=>$componentstr, 'item'=>$typesstr, 'error'=>false); } return $status; }

Show
Daniele Cordella added a comment - Thanks David. I also found that the set of three functions: newmodule_reset_course_form_definition, workshop_reset_course_form_defaults and workshop_reset_userdata described below, are still missing. /**
  • Called by course/reset.php
  • @param $mform form passed by reference */ function newmodule_reset_course_form_definition(&$mform) {
$mform->addElement('header', ' newmoduleheader', get_string('modulenameplural', 'newmodule')); $mform->addElement('checkbox', 'reset_newmodule_all', get_string('resetnewmoduleall','newmodule')); } /**
  • Course reset form defaults. */ function workshop_reset_course_form_defaults($course) { return array('reset_newmodule_all'=>1); }
/**
  • This function is used by the reset_course_userdata function in moodlelib.
  • This function will remove all issued certificates from the specified course
  • @param $data the data submitted from the reset course.
  • @return array status array */ function workshop_reset_userdata($data) { global $CFG;
$componentstr = get_string('modulenameplural', 'newmodule'); $typesstr = get_string('resetcertificateall', 'newmodule'); $status = array() if (!empty($data->reset_newmodule_all)) { // purge tables and moodledata if needed $status[] = array('component'=>$componentstr, 'item'=>$typesstr, 'error'=>false); } return $status; }
Hide
Permalink
Daniele Cordella added a comment -

again David,
in mod_form.php
it is written:

//-------------------------------------------------------------------------------
// add standard elements, common to all modules
$this->standard_coursemodule_elements();

instead of the more complete:

//-------------------------------------------------------------------------------
// add standard elements, common to all modules
$features = new object();
$features->groups = false;
$features->groupings = false;
$features->groupmembersonly = true;
$this->standard_coursemodule_elements($features);

Show
Daniele Cordella added a comment - again David, in mod_form.php it is written: //------------------------------------------------------------------------------- // add standard elements, common to all modules $this->standard_coursemodule_elements(); instead of the more complete: //------------------------------------------------------------------------------- // add standard elements, common to all modules $features = new object(); $features->groups = false; $features->groupings = false; $features->groupmembersonly = true; $this->standard_coursemodule_elements($features);
Hide
Permalink
Daniele Cordella added a comment -

Ciao David (or Dan, maybe).
In the view.php of the NEWMODULE.ZIP it is missing the following snippet of code to prevent direct access to hidden resources.

/// If it's hidden then it doesn't show anything.
if (empty($cm->visible) and !has_capability('moodle/course:viewhiddenactivities', $context)) { notice(get_string("activityiscurrentlyhidden")); exit; }

Two seconds time issue!

Show
Daniele Cordella added a comment - Ciao David (or Dan, maybe). In the view.php of the NEWMODULE.ZIP it is missing the following snippet of code to prevent direct access to hidden resources. /// If it's hidden then it doesn't show anything. if (empty($cm->visible) and !has_capability('moodle/course:viewhiddenactivities', $context)) { notice(get_string("activityiscurrentlyhidden")); exit; } Two seconds time issue!
Hide
Permalink
David Mudrak added a comment -

Hi Daniele,

no, this sort of checking is done by require_login() once it is provided $cm object.

d.

Show
David Mudrak added a comment - Hi Daniele, no, this sort of checking is done by require_login() once it is provided $cm object. d.
Hide
Permalink
Daniele Cordella added a comment -

Thanks David. One more info I was missing. Thanks!!!

Show
Daniele Cordella added a comment - Thanks David. One more info I was missing. Thanks!!!
Hide
Permalink
Daniele Cordella added a comment -

Available in
http://moodle.org/mod/forum/discuss.php?d=148581
a tool to look for useless strings during newmodule development.

Show
Daniele Cordella added a comment - Available in http://moodle.org/mod/forum/discuss.php?d=148581 a tool to look for useless strings during newmodule development.
Hide
Permalink
Daniele Cordella added a comment -

the function "error" should be replaced with "print_error" even in NEW_MODULE for moodle 1.9
as written in moodle19/lib/weblib.php in the comments of the print_error function

Show
Daniele Cordella added a comment - the function "error" should be replaced with "print_error" even in NEW_MODULE for moodle 1.9 as written in moodle19/lib/weblib.php in the comments of the print_error function
Hide
Permalink
Daniele Cordella added a comment -

Ciao Dan.
I just found that...
in the just downloaded version of NEWMODULE for 1.9, the variable $return is not defined in lib.php at line 98

Ciao.

Show
Daniele Cordella added a comment - Ciao Dan. I just found that... in the just downloaded version of NEWMODULE for 1.9, the variable $return is not defined in lib.php at line 98 Ciao.
Hide
Permalink
David Mudrak added a comment -

Daniele, this is not actually a bug. Note that NEWMODULE is a scaffold and is not expected to work as-is. The developer should implement the function returning the requested object.

Show
David Mudrak added a comment - Daniele, this is not actually a bug. Note that NEWMODULE is a scaffold and is not expected to work as-is. The developer should implement the function returning the requested object.
Hide
Permalink
Daniele Cordella added a comment -

you are 50% right, David.
I may agree with you but, please, let me say that NEWMODULE has to default to a reasonable value, too.

Show
Daniele Cordella added a comment - you are 50% right, David. I may agree with you but, please, let me say that NEWMODULE has to default to a reasonable value, too.
Hide
Permalink
Dan Poltawski added a comment -

I'm unassigning this from me, and giving it to 'Nobody' - because i've done such a terrible job of doing it & it'd be clearer if someone else knew they should pick it up.

Sorry everyone..

Show
Dan Poltawski added a comment - I'm unassigning this from me, and giving it to 'Nobody' - because i've done such a terrible job of doing it & it'd be clearer if someone else knew they should pick it up. Sorry everyone..
Hide
Permalink
Frank Ralf added a comment -

JFTR
The code is also available at https://github.com/moodlehq/moodle-mod_newmodule

Show
Frank Ralf added a comment - JFTR The code is also available at https://github.com/moodlehq/moodle-mod_newmodule
Hide
Permalink
David Mudrak added a comment -

@Dan: there is definitely no need to apologize for anything! You did what you had time for - thanks a lot for that!

@Frank: yes, that git repo is actually considered the primary source of the module now. I expect the CVS version would die sometime (the sooner, the better)

Show
David Mudrak added a comment - @Dan: there is definitely no need to apologize for anything! You did what you had time for - thanks a lot for that! @Frank: yes, that git repo is actually considered the primary source of the module now. I expect the CVS version would die sometime (the sooner, the better)
Hide
Permalink
Anthony Borrow added a comment -

Yes, I am proposing that CVS could be disabled when support for Moodle 1.9 ends. I have been encouraging folks writing CONTRIB code for Moodle 2.0 to use some type of repository - preferably Git. Thanks Dan for all you have been able to do. I'll see if anyone at HQ might be interested in picking up maintaining NEWMODULE. Peace - Anthony

Show
Anthony Borrow added a comment - Yes, I am proposing that CVS could be disabled when support for Moodle 1.9 ends. I have been encouraging folks writing CONTRIB code for Moodle 2.0 to use some type of repository - preferably Git. Thanks Dan for all you have been able to do. I'll see if anyone at HQ might be interested in picking up maintaining NEWMODULE. Peace - Anthony
Hide
Permalink
Anthony Borrow added a comment -

I am re-assigning to see if anyone at HQ is interested in helping to keep NEWMODULE updated. Peace - Anthony

Show
Anthony Borrow added a comment - I am re-assigning to see if anyone at HQ is interested in helping to keep NEWMODULE updated. Peace - Anthony
Hide
Permalink
David Mudrak added a comment -

Anthony: I can continue with occasional updating newmodule, please feel free to set me as the component lead.

Show
David Mudrak added a comment - Anthony: I can continue with occasional updating newmodule, please feel free to set me as the component lead.
Hide
Permalink
Anthony Borrow added a comment -

Thanks David! That is very generous of you. Michael de Raadt is also interested in helping out some as well - looks like I mistakenly assigned it to him. I'll let you guys thumb wrestle for issues or at least tag team on them. Peace - Anthony

Show
Anthony Borrow added a comment - Thanks David! That is very generous of you. Michael de Raadt is also interested in helping out some as well - looks like I mistakenly assigned it to him. I'll let you guys thumb wrestle for issues or at least tag team on them. Peace - Anthony

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.