Details
Bug
Major
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!
Gliffy Diagrams
Issue Links
- has a non-specific relationship to
-
MDL-33783
Move CORE to use frankenstyle everywhere
-
- Closed
-
Activity
David: I think one key thing is that the majority of these have been introduced in this release, its not jsut general tidying.
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.
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.
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 
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.
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...
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.
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) {
|
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
moodle core is so awesome no code change had to be done.
this is up for peer review. good night!
Thanks Tim, it worked fine for me. up for integration review now.
Thanks Apu, integrated now
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()
|
This error does seem like it could be related to this change.
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?
I've pulled that in, thanks Apu.
(I also fixed minor whitespace issue)
Fred, could you retest, thanks?
Successful on master!
Note: I've created MDL-33783 about the task of moving to frankestyle everywhere at some point.
Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL
Ciao
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?