Moodle

Add support for custom context levels

Details

  • Type: New Feature New Feature
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.9, 2.0
  • Fix Version/s: None
  • Component/s: Roles / Access
  • Labels:
    None

Description

This issue is in reference to http://moodle.org/mod/forum/discuss.php?d=111362.
In brief, one of our Moodle extensions would benefit greatly from being able to use Moodle's roles and contexts system, and we would need to define our own context levels. However, we would like to be able to do this without having to differ from core in unmaintainable ways.
We believe that this would be helpful to many other Moodle partners and plugin developers, and some of our proposed changes may help to improve the maintainability and readability of core Moodle code.

Attachments

  1. File
    remove_switch_statements.diff
    16/Sep/11 3:06 AM
    65 kB
    Hubert Chathi
  2. File
    remove_switch_statements.diff
    17/Sep/09 3:11 AM
    67 kB
    Hubert Chathi
  3. File
    remove_switch_statements.diff
    28/Aug/09 11:06 PM
    56 kB
    Hubert Chathi
  4. File
    remove_switch_statements.diff
    12/Aug/09 12:24 AM
    59 kB
    Hubert Chathi
  5. File
    testaccesslib2.php
    28/Aug/09 11:06 PM
    14 kB
    Hubert Chathi

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Hubert Chathi added a comment -

remove_switch_statements.diff adds removes some of the huge switch statements, and does some other reorganizing of context level-specific functions into classes, as outlined in http://moodle.org/mod/forum/discuss.php?d=111362#p561990 . It also replaces the array in get_context_instance with the context_levels array defined in the patch.

This applies against Moodle 1.9, and so is missing changes to get_context_url and get_contextlevel_name which are only in Moodle 2.0, but they can be easily added once this patch is reviewed.

We did not change create_contexts – it is only used for upgrading from Moodle 1.6, or for the grade letter upgrade.

Show
Hubert Chathi added a comment - remove_switch_statements.diff adds removes some of the huge switch statements, and does some other reorganizing of context level-specific functions into classes, as outlined in http://moodle.org/mod/forum/discuss.php?d=111362#p561990 . It also replaces the array in get_context_instance with the context_levels array defined in the patch. This applies against Moodle 1.9, and so is missing changes to get_context_url and get_contextlevel_name which are only in Moodle 2.0, but they can be easily added once this patch is reviewed. We did not change create_contexts – it is only used for upgrading from Moodle 1.6, or for the grade letter upgrade.
Hide
Permalink
Tim Hunt added a comment -

The big problem you have is that a patch like this has not got any chance of being included in Moodle 1.9, and there are many changed in accesslib.php in Moodle 2.0, many more than you mention in your forum post. So, there is limited point us reviewing a 1.9 patch. However I had a look and spotted the following point:

Please write unit tests to demonstrate that your code actually works. I know accesslib should have unit tests already, but it doesn't. The only way we will get them is if we add them progressively as we work on different functions there. Recently change code is the most likely to be buggy, so it makes sense to concentrate new unit tests there.

I would call it context_level_base. I guess that is just a personal preference. However I like that convention with abstract base classes.

context_level::cleanup_contexts_sql relies on $this->table, but that field is not declared in context_level.

You seem to have copied huge chunks of code into each build_context_path method.

context_block, etc are bad names. Objects of these classes are not contexts, they are context-levels. I think the names should be block_context_level.

Don't use a global for $context_levels (and your choice of name there violates the coding guidelines in two ways.) Use a static field context_level_base::$all_levels, and wrap access to it in a function get_context_levels().

$pre_sql is another variable whose name violates the coding guidelines.

I am sure there are other problems, but that is enough for now.

Show
Tim Hunt added a comment - The big problem you have is that a patch like this has not got any chance of being included in Moodle 1.9, and there are many changed in accesslib.php in Moodle 2.0, many more than you mention in your forum post. So, there is limited point us reviewing a 1.9 patch. However I had a look and spotted the following point: Please write unit tests to demonstrate that your code actually works. I know accesslib should have unit tests already, but it doesn't. The only way we will get them is if we add them progressively as we work on different functions there. Recently change code is the most likely to be buggy, so it makes sense to concentrate new unit tests there. I would call it context_level_base. I guess that is just a personal preference. However I like that convention with abstract base classes. context_level::cleanup_contexts_sql relies on $this->table, but that field is not declared in context_level. You seem to have copied huge chunks of code into each build_context_path method. context_block, etc are bad names. Objects of these classes are not contexts, they are context-levels. I think the names should be block_context_level. Don't use a global for $context_levels (and your choice of name there violates the coding guidelines in two ways.) Use a static field context_level_base::$all_levels, and wrap access to it in a function get_context_levels(). $pre_sql is another variable whose name violates the coding guidelines. I am sure there are other problems, but that is enough for now.
Hide
Permalink
Hubert Chathi added a comment -

Thanks for the comments, Tim. I was expecting that I'd have to go a couple of rounds to get things like coding conventions and stylistic preferences down.

Show
Hubert Chathi added a comment - Thanks for the comments, Tim. I was expecting that I'd have to go a couple of rounds to get things like coding conventions and stylistic preferences down.
Hide
Permalink
Hubert Chathi added a comment -

I have attached a new patch (remove_switch_statements.diff), this time against Moodle 2.0. I have fixed all the issues mentioned by Tim. One deviation is that I have called the classes e.g. context_level_block instead of e.g. block_context_level as he suggested. I find the former makes more sense to me, but it's a minor thing, and I'd be willing to change it.

I've also attached the simpletest file that I've used to make sure that it works as expected. Since many things in accesslib require actual contexts, some of the tests hardcode return values that are specific to my own test site, and will fail. These parts are clearly marked with "XXX". I have run the tests on both my code and the current Moodle 2.0 code, and they produce the same results, except for test_context_level_exists (which tests code that is only present in my version, so will obviously fail), and test_get_child_contexts (due to a bug? in the Moodle code – see below).

In the process of creating this patch, I noticed two issues, which I believe are bugs in Moodle (and I will be filing new issues shortly):

  • get_child_contexts sometimes returns a record set instead of an array, which is what the documentation states it is. This breaks get_role_context_caps, since it passes the return value of get_child_contexts to array_keys. In addition, the record set has already been iterated through, and cannot be rewound.
  • fetch_context_capabilities for blocks and modules searches for records that have the component equal to "block/..." and "mod/..." respectively. However in Moodle 2.0, the format seems to have been changed to "block_..." and "mod_...". ("_" instead of "/".)
    Both of these issues are fixed in my patch.

One thing that I'm not entirely sure about, stylistically, regarding my patch: I added get_context_level and context_level_exists methods. I believe this is the best way to do things, rather than always fetching the whole contexts array using the get_all_context_levels method.

Show
Hubert Chathi added a comment - I have attached a new patch (remove_switch_statements.diff), this time against Moodle 2.0. I have fixed all the issues mentioned by Tim. One deviation is that I have called the classes e.g. context_level_block instead of e.g. block_context_level as he suggested. I find the former makes more sense to me, but it's a minor thing, and I'd be willing to change it. I've also attached the simpletest file that I've used to make sure that it works as expected. Since many things in accesslib require actual contexts, some of the tests hardcode return values that are specific to my own test site, and will fail. These parts are clearly marked with "XXX". I have run the tests on both my code and the current Moodle 2.0 code, and they produce the same results, except for test_context_level_exists (which tests code that is only present in my version, so will obviously fail), and test_get_child_contexts (due to a bug? in the Moodle code – see below). In the process of creating this patch, I noticed two issues, which I believe are bugs in Moodle (and I will be filing new issues shortly):
  • get_child_contexts sometimes returns a record set instead of an array, which is what the documentation states it is. This breaks get_role_context_caps, since it passes the return value of get_child_contexts to array_keys. In addition, the record set has already been iterated through, and cannot be rewound.
  • fetch_context_capabilities for blocks and modules searches for records that have the component equal to "block/..." and "mod/..." respectively. However in Moodle 2.0, the format seems to have been changed to "block_..." and "mod_...". ("_" instead of "/".) Both of these issues are fixed in my patch.
One thing that I'm not entirely sure about, stylistically, regarding my patch: I added get_context_level and context_level_exists methods. I believe this is the best way to do things, rather than always fetching the whole contexts array using the get_all_context_levels method.
Hide
Permalink
Hubert Chathi added a comment -

For reference, the two bugs that I mentioned above are filed in MDL-20211.

Another thing that I forgot to mention in my last comment, that I'm not sure about regarding my patch, is the handling of the $emptyclause parameter for the build_context_path method. Currently, I have $a as the table name, and I'm using eval to replace it with the correct table name, like get_string does, because sometimes it needs "{context}" as the table name, and sometimes it needs "ctx" is the table name. Another alternative is to put %s in the table name, and use sprintf to insert the correct table name. Or I could put "{context}" as the table name, and do a search-and-replace to replace it with "ctx" when necessary. Any of these options would be a fairly easy change.

Show
Hubert Chathi added a comment - For reference, the two bugs that I mentioned above are filed in MDL-20211. Another thing that I forgot to mention in my last comment, that I'm not sure about regarding my patch, is the handling of the $emptyclause parameter for the build_context_path method. Currently, I have $a as the table name, and I'm using eval to replace it with the correct table name, like get_string does, because sometimes it needs "{context}" as the table name, and sometimes it needs "ctx" is the table name. Another alternative is to put %s in the table name, and use sprintf to insert the correct table name. Or I could put "{context}" as the table name, and do a search-and-replace to replace it with "ctx" when necessary. Any of these options would be a fairly easy change.
Hide
Permalink
Hubert Chathi added a comment -

The create_context function also needs updating – it slipped through my initial assessment. I'll post a patch shortly that splits out that function into the context level classes.

Show
Hubert Chathi added a comment - The create_context function also needs updating – it slipped through my initial assessment. I'll post a patch shortly that splits out that function into the context level classes.
Hide
Permalink
Hubert Chathi added a comment -

updated patch, which also splits create_context into the context level classes

Show
Hubert Chathi added a comment - updated patch, which also splits create_context into the context level classes
Hide
Permalink
Hubert Chathi added a comment -

New patch, which applies against Moodle 2.1.

Show
Hubert Chathi added a comment - New patch, which applies against Moodle 2.1.

People

Vote (0)
Watch (4)

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.