Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5, DEV backlog
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      There are a lot of changes regarding courses and categories management, test can not cover all of them but here are the most important

      1. Create a new site
      2. Browse front page, courses view, manage courses, make sure there are no errors on empty site
      3. Add block courses list to frontpage, make sure it displays information correctly during the rest of the test
      4. Create several courses in Miscellaneous category
      5. Browse front page, courses view, manage courses, make sure there are no errors on site with only one category
      6. Add more categories and courses, move them, make invisible, etc
      7. Edit and delete categories
      8. Search courses
      9. Make some role (i.e. teachers) allowed to create course requests, create user with this role
      10. Make sure you can filter users by role in category in /admin/user.php
      11. Create a cohort in course category context
      12. Login as teacher, make sure you can create course request
      13. Login as admin/manager, make sure you can view, approve and decline course requests
      14. Create, update, delete course categories using web services
      Show
      There are a lot of changes regarding courses and categories management, test can not cover all of them but here are the most important Create a new site Browse front page, courses view, manage courses, make sure there are no errors on empty site Add block courses list to frontpage, make sure it displays information correctly during the rest of the test Create several courses in Miscellaneous category Browse front page, courses view, manage courses, make sure there are no errors on site with only one category Add more categories and courses, move them, make invisible, etc Edit and delete categories Search courses Make some role (i.e. teachers) allowed to create course requests, create user with this role Make sure you can filter users by role in category in /admin/user.php Create a cohort in course category context Login as teacher, make sure you can create course request Login as admin/manager, make sure you can view, approve and decline course requests Create, update, delete course categories using web services
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-38147-master
    • Rank:
      47973

      Description

      Create class coursecat for managing and displaying course categories

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          Hi Sam. Ready for you to peer review.
          Note that first three commits have different issue numbers - they are just picked up from other issues that block this one.

          The first two commits with this issue number are the most important to review - this is the coursecat class itself and all unittests.

          All other commits are just deprecating of global functions and using the coursecat class instead

          Show
          Marina Glancy added a comment - Hi Sam. Ready for you to peer review. Note that first three commits have different issue numbers - they are just picked up from other issues that block this one. The first two commits with this issue number are the most important to review - this is the coursecat class itself and all unittests. All other commits are just deprecating of global functions and using the coursecat class instead
          Hide
          Sam Hemelryk added a comment -

          Hi Marina

          I've begun to look at this now. Presently I've really just wrapped my head around how things have changed and have only really looked closely over lib/coursecat.php (as really that is the interesting file in this patch).
          I'm sharing the notes I've made so far, it'll be interesting to hear your thoughts on the points, feel free to argue/debate with me on them. No worries at all there.

          coursecat class

          • I don't think we should cache description, descriptionformat, visibleold, timemodified, or theme. Really description is the big one, avoiding that will save of the space used and will aid space restricted back-ends (think memcache etc). descriptionformat, visibleold, timemodified, and theme really just because they are not regularly used fields.
          • Caching context information against the category is tricky business. We need to ensure if this is done that marking the context dirty, and any other associated context actions purge the category cache as well. Unit tests would be a 110% requirement in order to prove any such efforts.
          • An alternative to having to implement IteratorArray would be to extend stdClass. The advantage in doing this is that we would be able to use this category in places where existing code is already type hinting a stdClass instance expecting the argument to be a result from the database. Not sure if this is the intention but surely it is worth aiming for. (This is something learnt from the modinfo switch).
          • I am really quite unsure about the use of a coursecat object with id=0 for control/access to top level courses. My fear hear is that such an object could be instantiated and then passed around (perhaps accidentally). I'm not entirely against the idea I just don't like the idea of mixing real objects with fake objects that exist just to access functionality. Perhaps a solution would be to create a static method on coursecat to return the top level categories and from them make it only possible to instantiate a coursecat object with id=0 internally. That should be a problem as get would just reject requests for get(0) and instruct people to use ::get_top_level_categories or something. Is there anything else that we use the top level category for that this change wouldn't allow for?
          • Don't use cache->has, cache->has_any, or cache->has_all. They are completely flawed methods that exist only because I wasn't bold enough to tell people they couldn't have them. Have a look at the phpdocs for those methods, the docs pages, or the readme for details why. I regret those methods existing and hope that they are never used in core, your code will look better when you aren't using them as well, the current use within course_cat::get for instance is flawed in that it if has returns true the code expects get will succeed. In fact the cache may change between the has request and the get request in which case you'll end up with $coursecat = false when the category does in fact exist. It is preferred to use get and then check the result.
          • coursecat::get phpdoc has a namespace \ on @return that should be removed.
          • I'm thinking that the third argument to the static get method should be $returnhidden, invisible isn't a term we use and it'd be nice not to introduce a new term here.
          • When creating $coursecat0 why not create a new stdClass and assign its properties, I think that would read clearer and certainly would be better for performance as casting can be slow (although it only happens once so performance would be negligible).
          • context_instance_preload_sql is a deprecated function (argue with Petr re that one )
          • get_all_ids shouldn't purge the cache after failing to retrieve the all record. That may lead to race conditions on a high load system.
          • get_all_ids should be checking $all[$record->parent] is an array before using it as such. **check me**
          • get_all is going to be really painful on a large installation. Surely we could bulk load from the database/cache.
          • same with get_children
          • cnt_all => count_all
          • can_delete_full, should that be preloading contexts when checking fetching them in order to call can_delete_course? Just thinking that the first thing that can_delete_course does is load the context so probably a good performance win there? This could easily be done later as well.
          • delete_full needs to be marked public.
          • Regarding the function names starting with underscores this is something we really try to avoid. We used to use them back in the PHP4 days to mark internal (protected/private) methods as scope wasn't available. However now we try to avoid it, exceptions are allowed if there is a very good reason to name it so. Looking at the hide method it appears you've done this to avoid calling purge and logging. I think perhaps the best approach here would be to make it an option of the function to purge the cache that is on by default. Then when you would call _hide you just set the third argument to false. This would allow bulk operation to happen easily.
          • Just noting that the coursecat cache definition would benefit greatly from the persistent option being set to true. I think I talked with Tim Hunt about setting it on by default at some point perhaps we should revisit that. Either way at present I would set that to true for sure.

          Caching.

          How is the core/coursecat cache used:

          • 'User'.$user->id: as key caches the categories visible to the user.
          • 'all': contains a mapping array with 0 containing all top level categories and then an id (the key) containing the sub categories as an array (the value).
          • $id: The category id mapping to the category record from the database stored in the cache.

          The use of the coursecat cache to store categories visible to a user.
          First up just to summarise this cache.
          get_all_visible caches visible categories for a user. It is an application cache. It checks that the category is visible. It retrieves the category via get so that it isn't return if it is hidden. and it checks to make sure its parent is visible.
          get calls is_uservisible for the category. is_uservisible checks the id is valid, the the category is visible or that the user has the view hidden categories capability.
          The cache will need to be purged under the following circumstances:

          • The category visibility changes.
          • The parent category visibility changes.
          • User capabilities change for the category or any contexts along the category context paths.

          The requirement of this side fo the cache is likely to end up being very large. As its persistent a site is likely to end up with a single cache entry for every user here that will only be removed when the cache is purged. This is undesirable as this information is only useful to an individual user. I feel this side of the coursecat cache should be separated out to a dedicated session cache.

          Performace.

          I'm just wondering what performance testing you've done on these changes Marina.
          I've done a little rudimentary testing myself and while I haven't delved deep into it at present it appears to me that performance has got a little worse.
          I tested just the front page of my peer-review site which has display set to combo-list, list of categories, and finally list of courses. The site itself is pretty much default Moodle and has only 21 categories and 53 courses.
          The big difference I noted was that with the patch applied the number of database reads increased by from, for both guests and admin users. Originally I was getting 229 as admin and 209 when not logged in. After this change I was getting 249 as admin and 229 when not logged in.
          For curiosity sake with the patch applied I disabled the coursecat cache to find out what the overall increase in reads with would be if. The result was 539 when not logged in.
          I think perhaps there is more investigation to be done here.

          The notes were already getting long so I tried to summarise some points for discussion/review. Once this has moved forward a bit more I'm happy to continue on with the review.
          Overall I really like that we are moving towards a class structure however I think performance really needs to be looked at here and there are still some structural issues to address I think.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina I've begun to look at this now. Presently I've really just wrapped my head around how things have changed and have only really looked closely over lib/coursecat.php (as really that is the interesting file in this patch). I'm sharing the notes I've made so far, it'll be interesting to hear your thoughts on the points, feel free to argue/debate with me on them. No worries at all there. coursecat class I don't think we should cache description, descriptionformat, visibleold, timemodified, or theme. Really description is the big one, avoiding that will save of the space used and will aid space restricted back-ends (think memcache etc). descriptionformat, visibleold, timemodified, and theme really just because they are not regularly used fields. Caching context information against the category is tricky business. We need to ensure if this is done that marking the context dirty, and any other associated context actions purge the category cache as well. Unit tests would be a 110% requirement in order to prove any such efforts. An alternative to having to implement IteratorArray would be to extend stdClass. The advantage in doing this is that we would be able to use this category in places where existing code is already type hinting a stdClass instance expecting the argument to be a result from the database. Not sure if this is the intention but surely it is worth aiming for. (This is something learnt from the modinfo switch). I am really quite unsure about the use of a coursecat object with id=0 for control/access to top level courses. My fear hear is that such an object could be instantiated and then passed around (perhaps accidentally). I'm not entirely against the idea I just don't like the idea of mixing real objects with fake objects that exist just to access functionality. Perhaps a solution would be to create a static method on coursecat to return the top level categories and from them make it only possible to instantiate a coursecat object with id=0 internally. That should be a problem as get would just reject requests for get(0) and instruct people to use ::get_top_level_categories or something. Is there anything else that we use the top level category for that this change wouldn't allow for? Don't use cache->has, cache->has_any, or cache->has_all. They are completely flawed methods that exist only because I wasn't bold enough to tell people they couldn't have them. Have a look at the phpdocs for those methods, the docs pages, or the readme for details why. I regret those methods existing and hope that they are never used in core, your code will look better when you aren't using them as well, the current use within course_cat::get for instance is flawed in that it if has returns true the code expects get will succeed. In fact the cache may change between the has request and the get request in which case you'll end up with $coursecat = false when the category does in fact exist. It is preferred to use get and then check the result. coursecat::get phpdoc has a namespace \ on @return that should be removed. I'm thinking that the third argument to the static get method should be $returnhidden, invisible isn't a term we use and it'd be nice not to introduce a new term here. When creating $coursecat0 why not create a new stdClass and assign its properties, I think that would read clearer and certainly would be better for performance as casting can be slow (although it only happens once so performance would be negligible). context_instance_preload_sql is a deprecated function (argue with Petr re that one ) get_all_ids shouldn't purge the cache after failing to retrieve the all record. That may lead to race conditions on a high load system. get_all_ids should be checking $all [$record->parent] is an array before using it as such. ** check me ** get_all is going to be really painful on a large installation. Surely we could bulk load from the database/cache. same with get_children cnt_all => count_all can_delete_full, should that be preloading contexts when checking fetching them in order to call can_delete_course? Just thinking that the first thing that can_delete_course does is load the context so probably a good performance win there? This could easily be done later as well. delete_full needs to be marked public. Regarding the function names starting with underscores this is something we really try to avoid. We used to use them back in the PHP4 days to mark internal (protected/private) methods as scope wasn't available. However now we try to avoid it, exceptions are allowed if there is a very good reason to name it so. Looking at the hide method it appears you've done this to avoid calling purge and logging. I think perhaps the best approach here would be to make it an option of the function to purge the cache that is on by default. Then when you would call _hide you just set the third argument to false. This would allow bulk operation to happen easily. Just noting that the coursecat cache definition would benefit greatly from the persistent option being set to true. I think I talked with Tim Hunt about setting it on by default at some point perhaps we should revisit that. Either way at present I would set that to true for sure. Caching. How is the core/coursecat cache used: 'User'.$user->id: as key caches the categories visible to the user. 'all': contains a mapping array with 0 containing all top level categories and then an id (the key) containing the sub categories as an array (the value). $id: The category id mapping to the category record from the database stored in the cache. The use of the coursecat cache to store categories visible to a user. First up just to summarise this cache. get_all_visible caches visible categories for a user. It is an application cache. It checks that the category is visible. It retrieves the category via get so that it isn't return if it is hidden. and it checks to make sure its parent is visible. get calls is_uservisible for the category. is_uservisible checks the id is valid, the the category is visible or that the user has the view hidden categories capability. The cache will need to be purged under the following circumstances: The category visibility changes. The parent category visibility changes. User capabilities change for the category or any contexts along the category context paths. The requirement of this side fo the cache is likely to end up being very large. As its persistent a site is likely to end up with a single cache entry for every user here that will only be removed when the cache is purged. This is undesirable as this information is only useful to an individual user. I feel this side of the coursecat cache should be separated out to a dedicated session cache. Performace. I'm just wondering what performance testing you've done on these changes Marina. I've done a little rudimentary testing myself and while I haven't delved deep into it at present it appears to me that performance has got a little worse. I tested just the front page of my peer-review site which has display set to combo-list, list of categories, and finally list of courses. The site itself is pretty much default Moodle and has only 21 categories and 53 courses. The big difference I noted was that with the patch applied the number of database reads increased by from, for both guests and admin users. Originally I was getting 229 as admin and 209 when not logged in. After this change I was getting 249 as admin and 229 when not logged in. For curiosity sake with the patch applied I disabled the coursecat cache to find out what the overall increase in reads with would be if. The result was 539 when not logged in. I think perhaps there is more investigation to be done here. The notes were already getting long so I tried to summarise some points for discussion/review. Once this has moved forward a bit more I'm happy to continue on with the review. Overall I really like that we are moving towards a class structure however I think performance really needs to be looked at here and there are still some structural issues to address I think. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Hi Sam,

          thanks for your thorough review!

          Some things I agree with, some not We just had a two-hours discussion so I post here basically resume.

          First of all, I added three commits:

          • change small coding things such as using cache::has, \ namespace, rename parameter, unnecessary cache purging, cnt_ -> count_, cache persistent, preloaded contexts in can_delete_full(). Also I removed function get_all() completely as unnecessary
          • removing the function coursecat::get_all_visible() completely, it was only used in deprecated functions
          • renaming _hide(), _show() and _change_parent() to hide_raw(), show_raw() and change_parent_raw(). I don't agree with adding additional parameter. In fact all functions performing bulk actions on course categories should be inside this class as static methods, in this case they can access protected methods.

          I gave a lot of thoughts to changing what and how is cached, I'm going to try that but the API won't change.

          I really don't agree with extending stdClass. If developer wants to modify object or add new parameters - he is welcome to create a copy. But all serious code dealing with categories must be put inside the same library. Also by making this class extending stdClass we would allow developer not only to add but also to modify properties and this can be dangerous when calling the methods afterwards.

          Fake 0-category is quite convenient on browsing pages because we don't have to constantly make if's such as if id==0 => call one method to get children, if id!=0 - call another.

          Show
          Marina Glancy added a comment - Hi Sam, thanks for your thorough review! Some things I agree with, some not We just had a two-hours discussion so I post here basically resume. First of all, I added three commits: change small coding things such as using cache::has, \ namespace, rename parameter, unnecessary cache purging, cnt_ -> count_, cache persistent, preloaded contexts in can_delete_full(). Also I removed function get_all() completely as unnecessary removing the function coursecat::get_all_visible() completely, it was only used in deprecated functions renaming _hide(), _show() and _change_parent() to hide_raw(), show_raw() and change_parent_raw(). I don't agree with adding additional parameter. In fact all functions performing bulk actions on course categories should be inside this class as static methods, in this case they can access protected methods. I gave a lot of thoughts to changing what and how is cached, I'm going to try that but the API won't change. I really don't agree with extending stdClass. If developer wants to modify object or add new parameters - he is welcome to create a copy. But all serious code dealing with categories must be put inside the same library. Also by making this class extending stdClass we would allow developer not only to add but also to modify properties and this can be dangerous when calling the methods afterwards. Fake 0-category is quite convenient on browsing pages because we don't have to constantly make if's such as if id==0 => call one method to get children, if id!=0 - call another.
          Hide
          Marina Glancy added a comment -

          Hi Sam.
          I've been thinking a lot about what and how to cache and decided that it does not make much sense to cache a lot of individual course categories. To retrieve a course category and it's context from DB is one simple indexed DB query. Caching is not going to save us anything.

          Also I was thinking about use cases. What do we need from category tree:

          • in course management: retrieve one or several categories we perform a particular action on (to be honest, rare case)
          • in viewing list of courses and categories: retrieve one category and it's subcategories sorted by some field

          In both cases we don't retrieve the same category over and over again during one request.

          Also, list of subcategories is almost always the same for all users. Exceptions are when some categories are hidden, different users may see all, none or some of hidden categories. But again, the most categories are visible in which case we don't even need to load the context - everybody can see it.

          So this is what I decided to cache.
          On application level:

          • For each category the list of it's children ids
          • For each category that has children - list of it's hidden children ids (usually empty or short).

          For each user (session level):

          • For each category that has hidden children - list of children ids that are invisible for this particular user

          Also I'd like to cache the list of children sorted by some field other than sortorder but can't decide whether to cache on app or on user level. At the moment I've implemented it cached in session but maybe it should be cached on app level.

          Function coursecat::get_children() should have parameters such as sort, offset, limit.
          When we need to request children, we can look at [up to] three cached values - all children, if any - all hidden children, if any - all children invisible to the current user. Then we compute difference of arrays, splice with offset and limit and retrieve records only for the categories that need to be displayed. And there is no point in caching the list of the children.

          Also I still cache particular category retrieved with coursecat::get() but now in session cache and only for the main requested category, not for every retrieved child.

          What do you think?

          Show
          Marina Glancy added a comment - Hi Sam. I've been thinking a lot about what and how to cache and decided that it does not make much sense to cache a lot of individual course categories. To retrieve a course category and it's context from DB is one simple indexed DB query. Caching is not going to save us anything. Also I was thinking about use cases. What do we need from category tree: in course management: retrieve one or several categories we perform a particular action on (to be honest, rare case) in viewing list of courses and categories: retrieve one category and it's subcategories sorted by some field In both cases we don't retrieve the same category over and over again during one request. Also, list of subcategories is almost always the same for all users. Exceptions are when some categories are hidden, different users may see all, none or some of hidden categories. But again, the most categories are visible in which case we don't even need to load the context - everybody can see it. So this is what I decided to cache. On application level: For each category the list of it's children ids For each category that has children - list of it's hidden children ids (usually empty or short). For each user (session level): For each category that has hidden children - list of children ids that are invisible for this particular user Also I'd like to cache the list of children sorted by some field other than sortorder but can't decide whether to cache on app or on user level. At the moment I've implemented it cached in session but maybe it should be cached on app level. Function coursecat::get_children() should have parameters such as sort, offset, limit. When we need to request children, we can look at [up to] three cached values - all children, if any - all hidden children, if any - all children invisible to the current user. Then we compute difference of arrays, splice with offset and limit and retrieve records only for the categories that need to be displayed. And there is no point in caching the list of the children. Also I still cache particular category retrieved with coursecat::get() but now in session cache and only for the main requested category, not for every retrieved child. What do you think?
          Hide
          Marina Glancy added a comment -

          Hi Sam, sending it for peer review to you again

          Show
          Marina Glancy added a comment - Hi Sam, sending it for peer review to you again
          Hide
          Marina Glancy added a comment -

          There are two caches now:

          • 'coursecattree' is application level and is purged when anything in categories tree is changed. It caches only ids of children of each category (all and hidden).
          • 'coursecat' is session level, it is purged when anything is changed in either categories tree or courses. But also it has additional EXP keys for some listings because not all changes can be traced. It stores lists of categories accessible by current user. Also it caches for a short time list of ids of courses in category or in search results, so in case of pagination we don't need to re-query in order to display the next page. It caches category information but this is mainly used for retrieving category name in search results.

          Functions get_courses() and search_courses() retrieve only the needed records (as specified by options 'offset', 'limit', 'sort'), they also can retrieve list of course contacts. This is where I was stuck. The query from get_courses_wmanagers() can not work any more because it returns the users not enroled in the course. But I can't build a nice query that will both bulk load users for several courses and change their enrolments. I made a separate commit for that, want to ask Petr to help

          Show
          Marina Glancy added a comment - There are two caches now: 'coursecattree' is application level and is purged when anything in categories tree is changed. It caches only ids of children of each category (all and hidden). 'coursecat' is session level, it is purged when anything is changed in either categories tree or courses. But also it has additional EXP keys for some listings because not all changes can be traced. It stores lists of categories accessible by current user. Also it caches for a short time list of ids of courses in category or in search results, so in case of pagination we don't need to re-query in order to display the next page. It caches category information but this is mainly used for retrieving category name in search results. Functions get_courses() and search_courses() retrieve only the needed records (as specified by options 'offset', 'limit', 'sort'), they also can retrieve list of course contacts. This is where I was stuck. The query from get_courses_wmanagers() can not work any more because it returns the users not enroled in the course. But I can't build a nice query that will both bulk load users for several courses and change their enrolments. I made a separate commit for that, want to ask Petr to help
          Hide
          Marina Glancy added a comment -

          Sam, I added commit with make_categories_list().

          I started testing performance (together with MDL-37009) and hit some strange issue when number of DB queries for the front page sometimes jumps from 200 to 1000 in this branch. The reason appeared that the function print_course() in /course/lib.php does not apply course context when it formats the course name and description. In my renderer I apply the course context, which means that filters are not cached and it results in huge number of queries. So for the tests I commented out context option in format_string and format_name. I plan to discuss it on dev meeting tonight, maybe course context in format_text is not needed in this case - course summary never appears on course page. I will suggest to apply course category context.

          Also I set $CFG->coursecontact to none because the query to get course contacts changed. After that I can see that all performance numbers (time, RAM, DB read/writes, etc.) are about the same comparing to master. They become a little less in my branch in case of paginated results (such as search).

          Show
          Marina Glancy added a comment - Sam, I added commit with make_categories_list(). I started testing performance (together with MDL-37009 ) and hit some strange issue when number of DB queries for the front page sometimes jumps from 200 to 1000 in this branch. The reason appeared that the function print_course() in /course/lib.php does not apply course context when it formats the course name and description. In my renderer I apply the course context, which means that filters are not cached and it results in huge number of queries. So for the tests I commented out context option in format_string and format_name. I plan to discuss it on dev meeting tonight, maybe course context in format_text is not needed in this case - course summary never appears on course page. I will suggest to apply course category context. Also I set $CFG->coursecontact to none because the query to get course contacts changed. After that I can see that all performance numbers (time, RAM, DB read/writes, etc.) are about the same comparing to master. They become a little less in my branch in case of paginated results (such as search).
          Hide
          Marina Glancy added a comment -

          Linking to MDL-36898 where Petr change course contacts query for /course/info.php and /enrol/index.php

          Show
          Marina Glancy added a comment - Linking to MDL-36898 where Petr change course contacts query for /course/info.php and /enrol/index.php
          Hide
          Sam Hemelryk added a comment -

          Hi Marina,

          Looks real good thanks, really just a couple of minor things coming out of this really.

          coursecatlib.php

          • Just noting - presently there is no means to reset static $coursecat0.
          • Looking at coursecat::__get and course_in_list::get, I think perhaps it would be an idea to load all unloaded category fields when an field that has not been loaded is requested rather than loading just a single field.
            As we're fetching a single row things would be just as fast still, and we may save ourselves a query or few.
          • line 152 typo ALl => All.
          • Just noting - // Comments between functions are so-so, I tend to find they get lost. With comments like "implementing method from interface IteratorAggregate" I think they would be better added to the phpdoc for all functions they relate to. This is easier to find in both IDE's and doc systems.
          • __consturctur not phpdoc'd propertly.
          • Typo in exception unknowcategory, I see you didn't make this typo and it exists in several places so perhaps a new issue to fix this throughout Moodle.
          • create/update - could be using record_exists to check if the idnumber has been used. The strlen check should really be made before the check of idnumber as well, trivial though so no worries if left as is.
          • create - the array_intersect_key is a really complex way of building a simple three property object. No doubt it works but I think it would be much friendlier to just create a new updatedata object and set the three required properties.
          • compare_records => perhaps should be using collatorlib::asort_objects_by_property for locale aware sorting.
          • get_courses - first check of $list makes no sense. Neither does the second actually.
          • get_db_record ... will convert_to_array work with caching and unloaded fields. There don't appear to be any uses of this function, perhaps we just remove it?

          Deprecated functions used:

          • context_moved
          • context_instance_preload

          Caches:

          • coursecattree - fine.
          • coursecat - I think a couple of things need to be considered here.
            1. When caching the category it appears nothing user specific is involved in which case it would be 1000% beneficial to have that as a separate application cache.
            2. When caching course search results it appears nothing category specific is being included, if so then surely this would be better as a separate cache as well (although still a session cache).

          Commits: The last few commits you've made have started with either hyphens or spaces, would be nice to tidy that up.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina, Looks real good thanks, really just a couple of minor things coming out of this really. coursecatlib.php Just noting - presently there is no means to reset static $coursecat0. Looking at coursecat::__get and course_in_list::get, I think perhaps it would be an idea to load all unloaded category fields when an field that has not been loaded is requested rather than loading just a single field. As we're fetching a single row things would be just as fast still, and we may save ourselves a query or few. line 152 typo ALl => All. Just noting - // Comments between functions are so-so, I tend to find they get lost. With comments like "implementing method from interface IteratorAggregate" I think they would be better added to the phpdoc for all functions they relate to. This is easier to find in both IDE's and doc systems. __consturctur not phpdoc'd propertly. Typo in exception unknowcategory, I see you didn't make this typo and it exists in several places so perhaps a new issue to fix this throughout Moodle. create/update - could be using record_exists to check if the idnumber has been used. The strlen check should really be made before the check of idnumber as well, trivial though so no worries if left as is. create - the array_intersect_key is a really complex way of building a simple three property object. No doubt it works but I think it would be much friendlier to just create a new updatedata object and set the three required properties. compare_records => perhaps should be using collatorlib::asort_objects_by_property for locale aware sorting. get_courses - first check of $list makes no sense. Neither does the second actually. get_db_record ... will convert_to_array work with caching and unloaded fields. There don't appear to be any uses of this function, perhaps we just remove it? Deprecated functions used: context_moved context_instance_preload Caches: coursecattree - fine. coursecat - I think a couple of things need to be considered here. When caching the category it appears nothing user specific is involved in which case it would be 1000% beneficial to have that as a separate application cache. When caching course search results it appears nothing category specific is being included, if so then surely this would be better as a separate cache as well (although still a session cache). Commits: The last few commits you've made have started with either hyphens or spaces, would be nice to tidy that up. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          cache keys that I use in session cache

          • coursecat::get() : {$id} - stores coursecat object with this id
          • get_not_visible_children_ids() ic{$id} - stores list of ids of child categories of category $id that are not visible to current user (this may change if user is assigned some roles during the session)
          • get_children() : c{$id}:{$sort} - list of ids of the children categories, accessible to user and sorted
          • search_courses() : s-{$search & $sort}, scnt-{$search} - list of ids of courses that is a search result (and their count) ''(has EXP-)''
          • get_courses() : l-{$id}, l{$id}r, lcnt{$id}, lcnt{$id}-r - list of ids of courses accessible to user in category (or in category and all subcategories), and their count
          • make_categories_list() : catlist - full list of categories names visible to the user ''(has EXP-)''
          • make_categories_list() : catlist:{$capabilities} - list of categories ids where user has the capability ''(has EXP-)''

          So, we decided that {$id} we are moving to persistent request cache defined in params

          Actually when I look at all others, even those that do not have artificial EXP- prefix, they all can change when simultaneously admin assign user with some capabilities and the categories/courses that he can view change. So maybe we just add a 10-minutes ttl to this cache and not create any other?

          Show
          Marina Glancy added a comment - cache keys that I use in session cache coursecat::get() : {$id} - stores coursecat object with this id get_not_visible_children_ids() ic{$id} - stores list of ids of child categories of category $id that are not visible to current user (this may change if user is assigned some roles during the session) get_children() : c{$id}:{$sort} - list of ids of the children categories, accessible to user and sorted search_courses() : s-{$search & $sort} , scnt-{$search} - list of ids of courses that is a search result (and their count) ''(has EXP-)'' get_courses() : l-{$id} , l {$id} r , lcnt {$id} , lcnt {$id}-r - list of ids of courses accessible to user in category (or in category and all subcategories), and their count make_categories_list() : catlist - full list of categories names visible to the user ''(has EXP-)'' make_categories_list() : catlist:{$capabilities} - list of categories ids where user has the capability ''(has EXP-)'' So, we decided that {$id} we are moving to persistent request cache defined in params Actually when I look at all others, even those that do not have artificial EXP- prefix, they all can change when simultaneously admin assign user with some capabilities and the categories/courses that he can view change. So maybe we just add a 10-minutes ttl to this cache and not create any other?
          Hide
          Marina Glancy added a comment -

          Hi Sam,
          thanks for review!
          Some of the things you noticed were copy-pasted from another code or is a result of my code evolution like $list check in get_courses(), I've corrected them.
          There are a couple of things I do not agree or want to comment on:

          • resetting $coursecat0 makes no sense because it is very static object, it's basically a constant, there's just nothing to reset
          • I agree that coursecat::_get() can load all fields but not course_in_list::_get() - table course contains way too many fields we will never need to display list of courses, including modinfo and sectioncache. Actually I don't think anything except not preloaded summary may be requested there.
          • compare_records() - we discussed it with you in chat and decided to make a separate issue about it
          • get_db_record() - this is tricky. It is used in deprecated functions and also in one of the tests. I'll look how to get rid of it completely.
          • I made the whole cache coursecat having ttl 600s because everything can change when some permission are changed and I can't event-invalidate everything
          • commits that have hyphen prefix are the latest commits that are not organised yet, I will squash them or reword later
          Show
          Marina Glancy added a comment - Hi Sam, thanks for review! Some of the things you noticed were copy-pasted from another code or is a result of my code evolution like $list check in get_courses(), I've corrected them. There are a couple of things I do not agree or want to comment on: resetting $coursecat0 makes no sense because it is very static object, it's basically a constant, there's just nothing to reset I agree that coursecat::_ get() can load all fields but not course_in_list:: _get() - table course contains way too many fields we will never need to display list of courses, including modinfo and sectioncache. Actually I don't think anything except not preloaded summary may be requested there. compare_records() - we discussed it with you in chat and decided to make a separate issue about it get_db_record() - this is tricky. It is used in deprecated functions and also in one of the tests. I'll look how to get rid of it completely. I made the whole cache coursecat having ttl 600s because everything can change when some permission are changed and I can't event-invalidate everything commits that have hyphen prefix are the latest commits that are not organised yet, I will squash them or reword later
          Hide
          Marina Glancy added a comment -

          Just added a commit with small bug fix

          Show
          Marina Glancy added a comment - Just added a commit with small bug fix
          Hide
          Damyon Wiese added a comment -

          Hi Marina,

          You have removed the get_db_record function from the coursecat class - but there is still one usage in course/lib.php. (Breaking the course categories page for me).

          This code looks great by the way.

          • Damyon
          Show
          Damyon Wiese added a comment - Hi Marina, You have removed the get_db_record function from the coursecat class - but there is still one usage in course/lib.php. (Breaking the course categories page for me). This code looks great by the way. Damyon
          Hide
          Damyon Wiese added a comment -

          I bet you fixed it before I found it (looking now)

          Show
          Damyon Wiese added a comment - I bet you fixed it before I found it (looking now)
          Hide
          Damyon Wiese added a comment -

          Nope, you fixed a different bug - can you add a fix for the get_db_record bug?

          Thanks!

          Show
          Damyon Wiese added a comment - Nope, you fixed a different bug - can you add a fix for the get_db_record bug? Thanks!
          Hide
          Marina Glancy added a comment -

          ups, I changed in but only in the MDL-37009. Added commit

          Show
          Marina Glancy added a comment - ups, I changed in but only in the MDL-37009 . Added commit
          Hide
          Damyon Wiese added a comment -

          Thanks Marina,

          This is working for me now. Integrated to master.

          There is heaps of work in this patch and it looks like a big improvement - well done!

          Show
          Damyon Wiese added a comment - Thanks Marina, This is working for me now. Integrated to master. There is heaps of work in this patch and it looks like a big improvement - well done!
          Hide
          Adrian Greeve added a comment -

          I ran through the list of instructions provided.
          No errors found.
          Test passed.

          Show
          Adrian Greeve added a comment - I ran through the list of instructions provided. No errors found. Test passed.
          Hide
          Damyon Wiese added a comment -

          The units tests for this change are failing on MSSQL:

          dml_read_exception: Error reading from database (Argument data type ntext is invalid for argument 1 of len function.
          SELECT c.id,c.category,c.sortorder,c.shortname,c.fullname,c.idnumber,c.startdate,c.visible, LEN(c.summary) hassummary, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance
          FROM t_course c
          JOIN t_context ctx ON c.id = ctx.instanceid AND ctx.contextlevel = ?
          WHERE c.id <> ? AND c.category = ? ORDER BY c.sortorder
          [array (
          0 => 50,
          1 => '1',
          2 => '2',
          )])

          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/moodle_database.php:426
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/mssql_native_moodle_database.php:256
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/mssql_native_moodle_database.php:713
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/mssql_native_moodle_database.php:743
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/coursecatlib.php:747
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/coursecatlib.php:1188
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/tests/coursecatlib_test.php:377
          /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/phpunit/classes/advanced_testcase.php:76

          To re-run:
          ./vendor/bin/phpunit coursecatlib_testcase lib/tests/coursecatlib_test.php

          Show
          Damyon Wiese added a comment - The units tests for this change are failing on MSSQL: dml_read_exception: Error reading from database (Argument data type ntext is invalid for argument 1 of len function. SELECT c.id,c.category,c.sortorder,c.shortname,c.fullname,c.idnumber,c.startdate,c.visible, LEN(c.summary) hassummary, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance FROM t_course c JOIN t_context ctx ON c.id = ctx.instanceid AND ctx.contextlevel = ? WHERE c.id <> ? AND c.category = ? ORDER BY c.sortorder [array ( 0 => 50, 1 => '1', 2 => '2', )]) /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/moodle_database.php:426 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/mssql_native_moodle_database.php:256 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/mssql_native_moodle_database.php:713 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/dml/mssql_native_moodle_database.php:743 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/coursecatlib.php:747 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/coursecatlib.php:1188 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/tests/coursecatlib_test.php:377 /home/damyonw/Documents/Moodle/instances/im_mssql/moodle/lib/phpunit/classes/advanced_testcase.php:76 To re-run: ./vendor/bin/phpunit coursecatlib_testcase lib/tests/coursecatlib_test.php
          Hide
          Damyon Wiese added a comment -

          Unit test needs fixing.

          Show
          Damyon Wiese added a comment - Unit test needs fixing.
          Hide
          Damyon Wiese added a comment -

          I have MSSQL setup so I'll look at adding a patch for this.

          Show
          Damyon Wiese added a comment - I have MSSQL setup so I'll look at adding a patch for this.
          Hide
          Marina Glancy added a comment - - edited

          I think that our mssql dml needs fixing,
          function mssql_native_moodle_database::sql_length()

          http://www.mssqltips.com/sqlservertip/1188/how-to-get-length-of-text-ntext-and-image-columns-in-sql-server/

          Show
          Marina Glancy added a comment - - edited I think that our mssql dml needs fixing, function mssql_native_moodle_database::sql_length() http://www.mssqltips.com/sqlservertip/1188/how-to-get-length-of-text-ntext-and-image-columns-in-sql-server/
          Hide
          Michael de Raadt added a comment -

          I had similar errors reported.

          MSSQL (native in Windows)...

          2) coursecatlib_testcase::test_create_coursecat
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:90
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          3) coursecatlib_testcase::test_visibility
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:138
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          4) coursecatlib_testcase::test_update
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:233
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          5) coursecatlib_testcase::test_get_search_courses
          dml_read_exception: Error reading from database (SQLState: 42000<br>
          Error Code: 8116<br>
          Message: [Microsoft][SQL Server Native Client 11.0][SQL Server]Argument data type ntext is invalid for argument 1 of len function.<br>
          
          SELECT c.id,c.category,c.sortorder,c.shortname,c.fullname,c.idnumber,c.startdate,c.visible, LEN(c.summary) hassummary, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance
                          FROM phpu_course c
                          JOIN phpu_context ctx ON c.id = ctx.instanceid AND ctx.contextlevel = '50'
                          WHERE c.id <> '1' AND c.category = '2' ORDER BY c.sortorder
          [array (
            0 => 50,
            1 => '1',
            2 => '2',
          )])
          
          D:\xampp\htdocs\master_integration\lib\dml\moodle_database.php:426
          D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:260
          D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:367
          D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:779
          D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:827
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:747
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:1188
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:377
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          6) core_course_external_testcase::test_get_categories
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:221
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test.php
          
          7) core_course_external_testcase::test_update_categories
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:290
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test.php
          
          Show
          Michael de Raadt added a comment - I had similar errors reported. MSSQL (native in Windows)... 2) coursecatlib_testcase::test_create_coursecat Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:90 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 3) coursecatlib_testcase::test_visibility Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:138 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 4) coursecatlib_testcase::test_update Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:233 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 5) coursecatlib_testcase::test_get_search_courses dml_read_exception: Error reading from database (SQLState: 42000<br> Error Code: 8116<br> Message: [Microsoft][SQL Server Native Client 11.0][SQL Server]Argument data type ntext is invalid for argument 1 of len function.<br> SELECT c.id,c.category,c.sortorder,c.shortname,c.fullname,c.idnumber,c.startdate,c.visible, LEN(c.summary) hassummary, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, ctx.instanceid AS ctxinstance FROM phpu_course c JOIN phpu_context ctx ON c.id = ctx.instanceid AND ctx.contextlevel = '50' WHERE c.id <> '1' AND c.category = '2' ORDER BY c.sortorder [array ( 0 => 50, 1 => '1', 2 => '2', )]) D:\xampp\htdocs\master_integration\lib\dml\moodle_database.php:426 D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:260 D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:367 D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:779 D:\xampp\htdocs\master_integration\lib\dml\sqlsrv_native_moodle_database.php:827 D:\xampp\htdocs\master_integration\lib\coursecatlib.php:747 D:\xampp\htdocs\master_integration\lib\coursecatlib.php:1188 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:377 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 6) core_course_external_testcase::test_get_categories Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:221 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test.php 7) core_course_external_testcase::test_update_categories Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:290 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test.php
          Hide
          Michael de Raadt added a comment -

          PostgreSQL had some comment errors...

          2) coursecatlib_testcase::test_create_coursecat
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:90
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          3) coursecatlib_testcase::test_visibility
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:138
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          4) coursecatlib_testcase::test_update
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:233
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          
          5) core_course_external_testcase::test_get_categories
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:221
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test.
          php
          
          6) core_course_external_testcase::test_update_categories
          Array to string conversion
          
          D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126
          D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:290
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test.
          php
          
          Show
          Michael de Raadt added a comment - PostgreSQL had some comment errors... 2) coursecatlib_testcase::test_create_coursecat Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:90 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 3) coursecatlib_testcase::test_visibility Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:138 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 4) coursecatlib_testcase::test_update Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:233 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php 5) core_course_external_testcase::test_get_categories Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:221 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test. php 6) core_course_external_testcase::test_update_categories Array to string conversion D:\xampp\htdocs\master_integration\lib\coursecatlib.php:126 D:\xampp\htdocs\master_integration\course\tests\externallib_test.php:290 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit core_course_external_testcase course\tests\externallib_test. php
          Hide
          Michael de Raadt added a comment - - edited

          Similar errors have come up in MySQL. Marina pointed out that this is probably not DB dependant, but could be related to different versions of PHP. She suggests a quick change that I will try and report back on. There is still the one LEN() SQL issue that Marina is dealing with.

          Show
          Michael de Raadt added a comment - - edited Similar errors have come up in MySQL. Marina pointed out that this is probably not DB dependant, but could be related to different versions of PHP. She suggests a quick change that I will try and report back on. There is still the one LEN() SQL issue that Marina is dealing with.
          Hide
          Marina Glancy added a comment -

          oh you windows users.... this is php error, not DB-related. I pushed one commit for fixing MSSQL LEN() problems and ready to push another one fixing your php, Michael

          Show
          Marina Glancy added a comment - oh you windows users.... this is php error, not DB-related. I pushed one commit for fixing MSSQL LEN() problems and ready to push another one fixing your php, Michael
          Hide
          Marina Glancy added a comment -

          added second commit fixing windows php array_diff() behaviour;
          and third commit for unittests that has first two asserts for search_courses() that should pass on all databases and the last two may fail on some case-sensitive or lanugage-sensitive databases like MSSQL

          Show
          Marina Glancy added a comment - added second commit fixing windows php array_diff() behaviour; and third commit for unittests that has first two asserts for search_courses() that should pass on all databases and the last two may fail on some case-sensitive or lanugage-sensitive databases like MSSQL
          Hide
          Damyon Wiese added a comment -

          Thanks Marina,

          I've integrated those extra commits now to master - it would be good to get a retest of this issue for regressions. (Will ping David)

          Show
          Damyon Wiese added a comment - Thanks Marina, I've integrated those extra commits now to master - it would be good to get a retest of this issue for regressions. (Will ping David)
          Hide
          Damyon Wiese added a comment -

          (Note - there are 2 new failed asserts for MSSQL in this patch - it is related to collation/case sensitive/non-latin).

          Show
          Damyon Wiese added a comment - (Note - there are 2 new failed asserts for MSSQL in this patch - it is related to collation/case sensitive/non-latin).
          Hide
          Michael de Raadt added a comment - - edited

          I don't get any errors any more.

          I get one extra fail (on top of the two collation ones).

          3) coursecatlib_testcase::test_get_search_courses
          Failed asserting that two arrays are equal.
          --- Expected
          +++ Actual
          @@ @@
           Array (
          -    0 => '4'
          -    1 => '7'
           )
          
          D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:426
          D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76
          
          To re-run:
           \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php
          

          This happens in MSSQL only. The other DBs don't show this fail.

          Show
          Michael de Raadt added a comment - - edited I don't get any errors any more. I get one extra fail (on top of the two collation ones). 3) coursecatlib_testcase::test_get_search_courses Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ Array ( - 0 => '4' - 1 => '7' ) D:\xampp\htdocs\master_integration\lib\tests\coursecatlib_test.php:426 D:\xampp\htdocs\master_integration\lib\phpunit\classes\advanced_testcase.php:76 To re-run: \xampp\php\phpunit coursecatlib_testcase lib\tests\coursecatlib_test.php This happens in MSSQL only. The other DBs don't show this fail.
          Hide
          Michael de Raadt added a comment -

          I'm passing this as the functionality tested by Adrian passed. The errors introduced by the new unit tests are now resolved. There is one new unit test fail (reported as MDL-38754), but I don't think that should prevent this code from being integrated.

          Show
          Michael de Raadt added a comment - I'm passing this as the functionality tested by Adrian passed. The errors introduced by the new unit tests are now resolved. There is one new unit test fail (reported as MDL-38754 ), but I don't think that should prevent this code from being integrated.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: