Moodle
  1. Moodle
  2. MDL-33400

Activity modules are an exception to the rule when it comes to function/activity names

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      please re-run the test in MDL-22504 for drag and drop testing. especially for file, folder, page, and url activities.

      • test mod_assign : uploading files and downloading files from its assignments. (testing any single assignment type is enough)
      • test mod_quiz : create an essay question and download it.
      • test mod_data : file uploading to an entry and downloading it again.
      • test mod_assignment(2.2) : file uploading to a file type assignment and downloading.

      (the calls to plugin_callback() and get_plugin_list_with_function() already work fine with 'mod' so this is just resgression testing)

      Show
      please re-run the test in MDL-22504 for drag and drop testing. especially for file, folder, page, and url activities. test mod_assign : uploading files and downloading files from its assignments. (testing any single assignment type is enough) test mod_quiz : create an essay question and download it. test mod_data : file uploading to an entry and downloading it again. test mod_assignment(2.2) : file uploading to a file type assignment and downloading. (the calls to plugin_callback() and get_plugin_list_with_function() already work fine with 'mod' so this is just resgression testing)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      41274

      Description

      Activity modules are an exception to the rule when it comes to function/activity names.

      http://docs.moodle.org/dev/Activity_modules

      Currently however I see that a lot of functions and classes prefixed with mod_ have crept into lib.php:

      # grep "function mod_" */lib.php
      assign/lib.php:function mod_assign_get_file_areas($course, $cm, $context) {
      assign/lib.php:function mod_assign_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
      assignment/lib.php:function mod_assignment_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
      data/lib.php:function mod_data_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
      folder/lib.php:function mod_folder_dndupload_register() {
      folder/lib.php:function mod_folder_dndupload_handle($uploadinfo) {
      glossary/lib.php:function mod_glossary_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
      page/lib.php:function mod_page_dndupload_register() {
      page/lib.php:function mod_page_dndupload_handle($uploadinfo) {
      quiz/lib.php:function mod_quiz_question_pluginfile($course, $context, $component,
      resource/lib.php:function mod_resource_dndupload_register() {
      resource/lib.php:function mod_resource_dndupload_handle($uploadinfo) {
      url/lib.php:function mod_url_dndupload_register() {
      url/lib.php:function mod_url_dndupload_handle($uploadinfo) {
      
      # grep "class mod_" */lib.php
      assignment/lib.php:class mod_assignment_grading_form extends moodleform {
      

      This makes the situation slightly insane for developers. I'd love to change everything to mod_ but that is just not an option right now. I think we need to change it back.

      This means:

      1) Renaming the functions for one callback type at a time.
      2) Fixing the calling code looking for these prefixes.

      Here is a detailed list of what I think we need to fix:

      1) Fix all _get_file_areas (assign)
      2) Fix all _get_file_info (assign, assignment, data, glossary)
      3) Fix all _dndupload_register (folder, page, resource, url) and calling code!
      4) Fix all _dndupload_handle (folder, page, resource, url) and calling code!

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          Hmm, does this really worth it given how close we are to the release now? Could we instead announce big mod API changes in 2.5 and gradually upgrade everything in the next 12 months?

          Show
          David Mudrak added a comment - Hmm, does this really worth it given how close we are to the release now? Could we instead announce big mod API changes in 2.5 and gradually upgrade everything in the next 12 months?
          Hide
          Dan Poltawski added a comment -

          David: I think one key thing is that the majority of these have been introduced in this release, its not jsut general tidying.

          Show
          Dan Poltawski added a comment - David: I think one key thing is that the majority of these have been introduced in this release, its not jsut general tidying.
          Hide
          David Mudrak added a comment -

          Right. I just think that if the long-term goal is to have consistent frankenstyle everywhere, then new API should already respect that and legacy code is the one to be modified. I see no point on spending valuable time on refactoring the new code to respect habits that should be marked as deprecated. I see Martin's point re the importance of the consistency. But at the end, consistency is about using frankenstyle everywhere, with no exceptions.

          Show
          David Mudrak added a comment - Right. I just think that if the long-term goal is to have consistent frankenstyle everywhere, then new API should already respect that and legacy code is the one to be modified. I see no point on spending valuable time on refactoring the new code to respect habits that should be marked as deprecated. I see Martin's point re the importance of the consistency. But at the end, consistency is about using frankenstyle everywhere, with no exceptions.
          Hide
          Martin Dougiamas added a comment -

          One look at these modules reveals a disorganised mess (created in the last 6 months), one that is easy to clean up and makes it easier for people building on the 2.x platform. The exception for activities has never changed, so this is clearly a bug to me.

          The other path is to do a wholesale upgrade of ALL functions in those files, and also make the calling code support both names for backward compatibility. That to me seems like a waste of time right now. I'd rather put that energy into a really proper API rewrite later moving to classes/methods etc.

          Show
          Martin Dougiamas added a comment - One look at these modules reveals a disorganised mess (created in the last 6 months), one that is easy to clean up and makes it easier for people building on the 2.x platform. The exception for activities has never changed, so this is clearly a bug to me. The other path is to do a wholesale upgrade of ALL functions in those files, and also make the calling code support both names for backward compatibility. That to me seems like a waste of time right now. I'd rather put that energy into a really proper API rewrite later moving to classes/methods etc.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think I'm with Martin here.

          +1 to ban any "mod_" from all modules and keep the "exceptional handling" working for them.

          The only drawback I can imagine is that we cannot have plugin types in collision with activity names (so we cannot have one "feedback" plugintype, for example), but I don't think that's a big problem really.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think I'm with Martin here. +1 to ban any "mod_" from all modules and keep the "exceptional handling" working for them. The only drawback I can imagine is that we cannot have plugin types in collision with activity names (so we cannot have one "feedback" plugintype, for example), but I don't think that's a big problem really. Ciao
          Hide
          Dan Poltawski added a comment -

          The only drawback I can imagine is that we cannot have plugin types in collision with activity names (so we cannot have one "feedback" plugintype, for example), but I don't think that's a big problem really.

          We already have that problem? I don't think Martin is suggesting changing anything here other than removing the inconsistencies currently in lib.php files.

          Show
          Dan Poltawski added a comment - The only drawback I can imagine is that we cannot have plugin types in collision with activity names (so we cannot have one "feedback" plugintype, for example), but I don't think that's a big problem really. We already have that problem? I don't think Martin is suggesting changing anything here other than removing the inconsistencies currently in lib.php files.
          Hide
          Petr Škoda added a comment -

          The problem here is that we keep inventing new core subsystems which break existing modules with the same name - this is not a theoretical problem, it happened already. Anybody who votes against mod_ prefix should imho not be allowed to add new core subsystems or at least personally deal with any conflicts with contrib modules...

          Show
          Petr Škoda added a comment - The problem here is that we keep inventing new core subsystems which break existing modules with the same name - this is not a theoretical problem, it happened already. Anybody who votes against mod_ prefix should imho not be allowed to add new core subsystems or at least personally deal with any conflicts with contrib modules...
          Hide
          Dan Poltawski added a comment -

          Again, I don't think the suggestion is to change anything here. As far as I can tell, everyone is in agreement that we should move everything to mod_ eventually, this is just about making it consistent in lib.php until we eventually get to class mod_foo or whatever.

          Show
          Dan Poltawski added a comment - Again, I don't think the suggestion is to change anything here. As far as I can tell, everyone is in agreement that we should move everything to mod_ eventually, this is just about making it consistent in lib.php until we eventually get to class mod_foo or whatever.
          Hide
          Martin Dougiamas added a comment -

          Exactly. 12 functions in total. Look at this for example:

          ##grep _get_file_info */lib.php
          assign/lib.php:function mod_assign_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          assignment/lib.php:function mod_assignment_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          book/lib.php:function book_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          data/lib.php:function mod_data_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          folder/lib.php:function folder_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          forum/lib.php:function forum_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          glossary/lib.php:function mod_glossary_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          imscp/lib.php:function imscp_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          lesson/lib.php:function lesson_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          page/lib.php:function page_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          resource/lib.php:function resource_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          scorm/lib.php:function scorm_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          workshop/lib.php:function workshop_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          
          Show
          Martin Dougiamas added a comment - Exactly. 12 functions in total. Look at this for example: ##grep _get_file_info */lib.php assign/lib.php:function mod_assign_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { assignment/lib.php:function mod_assignment_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { book/lib.php:function book_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { data/lib.php:function mod_data_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { folder/lib.php:function folder_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { forum/lib.php:function forum_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { glossary/lib.php:function mod_glossary_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { imscp/lib.php:function imscp_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { lesson/lib.php:function lesson_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { page/lib.php:function page_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { resource/lib.php:function resource_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { scorm/lib.php:function scorm_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) { workshop/lib.php:function workshop_get_file_info($browser, $areas, $course, $cm, $context, $filearea, $itemid, $filepath, $filename) {
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, that's exactly what I was saying, take out all those recent "mod_" until we are ready to do the move seriously (2.5?).

          And yes, Dan, that happens since day 0, in fact I think it has happened already in th past.

          Because of this "mod exceptional usage" (not using frankenstyle), we cannot have other plugins/subsystems using any of the names of one module, because somehow the whole module "name-space" ("feedback_.") is reserved, and it will collide with one "feedback" plugin/subplugin/subsystem, which correct frankenstyle is also "feedback_.").

          Note I introduced that side-comment about collisions because I'm convinced that, in the long term, we should begin deprecating "the exceptional uses" (surely debugging some info, say for 2.4. And then, take rid of them for 2.5, for example).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, that's exactly what I was saying, take out all those recent "mod_" until we are ready to do the move seriously (2.5?). And yes, Dan, that happens since day 0, in fact I think it has happened already in th past. Because of this "mod exceptional usage" (not using frankenstyle), we cannot have other plugins/subsystems using any of the names of one module, because somehow the whole module "name-space" ("feedback_. ") is reserved, and it will collide with one "feedback" plugin/subplugin/subsystem, which correct frankenstyle is also "feedback_. "). Note I introduced that side-comment about collisions because I'm convinced that, in the long term, we should begin deprecating "the exceptional uses" (surely debugging some info, say for 2.4. And then, take rid of them for 2.5, for example). Ciao
          Hide
          Aparup Banerjee added a comment -

          moodle core is so awesome no code change had to be done.

          this is up for peer review. good night!

          Show
          Aparup Banerjee added a comment - moodle core is so awesome no code change had to be done. this is up for peer review. good night!
          Hide
          Tim Hunt added a comment -

          +1 from me, assuming you have tested it (I have not).

          Show
          Tim Hunt added a comment - +1 from me, assuming you have tested it (I have not).
          Hide
          Aparup Banerjee added a comment -

          Thanks Tim, it worked fine for me. up for integration review now.

          Show
          Aparup Banerjee added a comment - Thanks Tim, it worked fine for me. up for integration review now.
          Hide
          Dan Poltawski added a comment -

          Thanks Apu, integrated now

          Show
          Dan Poltawski added a comment - Thanks Apu, integrated now
          Hide
          Frédéric Massart added a comment -

          Failed on master.

          I don't know if this is related to this patch but file uploaded in an essay question in a quiz are not found.

          Sorry, the requested file could not be found
          
          Debug info: 
          Error code: filenotfound
          Stack trace:
          line 467 of /lib/setuplib.php: moodle_exception thrown
          line 1896 of /lib/filelib.php: call to print_error()
          line 1857 of /lib/questionlib.php: call to send_file_not_found()
          line 4007 of /lib/filelib.php: call to question_pluginfile()
          line 38 of /pluginfile.php: call to file_pluginfile()
          
          Show
          Frédéric Massart added a comment - Failed on master. I don't know if this is related to this patch but file uploaded in an essay question in a quiz are not found. Sorry, the requested file could not be found Debug info: Error code: filenotfound Stack trace: line 467 of /lib/setuplib.php: moodle_exception thrown line 1896 of /lib/filelib.php: call to print_error() line 1857 of /lib/questionlib.php: call to send_file_not_found() line 4007 of /lib/filelib.php: call to question_pluginfile() line 38 of /pluginfile.php: call to file_pluginfile()
          Hide
          Dan Poltawski added a comment -

          This error does seem like it could be related to this change.

          Show
          Dan Poltawski added a comment - This error does seem like it could be related to this change.
          Show
          Aparup Banerjee added a comment - Dan, here's the fix - https://github.com/nebgor/moodle/commit/0dd0f9637378d1e6b967fc49c57f6ebf87598007 ( https://github.com/nebgor/moodle/compare/mistress...MDL-33400 ) Fred, could you try it with that commit?
          Hide
          Dan Poltawski added a comment -

          I've pulled that in, thanks Apu.

          (I also fixed minor whitespace issue)

          Show
          Dan Poltawski added a comment - I've pulled that in, thanks Apu. (I also fixed minor whitespace issue)
          Hide
          Dan Poltawski added a comment -

          Fred, could you retest, thanks?

          Show
          Dan Poltawski added a comment - Fred, could you retest, thanks?
          Hide
          Frédéric Massart added a comment -

          Successful on master!

          Show
          Frédéric Massart added a comment - Successful on master!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note: I've created MDL-33783 about the task of moving to frankestyle everywhere at some point.

          Show
          Eloy Lafuente (stronk7) added a comment - Note: I've created MDL-33783 about the task of moving to frankestyle everywhere at some point.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: