Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please do not call get_users_roles() with $checkparentcontexts = true and $userids array not set. This combination causes large Moodle sites with lots of site-wide role assignemnts to run out of memory. #44

Closed
leevigraham opened this issue Aug 21, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@leevigraham
Copy link

commented Aug 21, 2018

The following error has appeared in our moodle installation v3.5.

We're using: Reengagement v3.4.1 - 2017122005

URL: mod/reengagement/view.php?id=276

Please do not call get_users_roles() with $checkparentcontexts = true and $userids array not set. This combination causes large Moodle sites with lots of site-wide role assignemnts to run out of memory.
line 2730 of /lib/accesslib.php: call to debugging()
line 247 of /mod/reengagement/classes/table/participants.php: call to get_users_roles()
line 226 of /mod/reengagement/view.php: call to mod_reengagement\table\participants->__construct()
@danmarsden

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

thanks for the report, feel free to help us work on improving this.

@leevigraham

This comment has been minimized.

Copy link
Author

commented Aug 22, 2018

@danmarsden, I'll take a look but this is my first Moodle install and I have 0 knowledge of how the moodle platform works under the hood :)

@RobertGoacher

This comment has been minimized.

Copy link

commented Oct 16, 2018

@leevigraham You should remove or edit that screenshot - you are exposing your users' personal data. 😱

@bfriesenvcc

This comment has been minimized.

Copy link

commented Jan 18, 2019

I just found and installed this same version on our Moodle 3.4.4 testing site and am getting the same error message. I'd LOVE to use this plugin, but can't risk it when I don't know what this means. :-(

@danmarsden

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

It's basically a warning to the developer that this function isn't very efficient and sites with large numbers of site-level role assignments may use a lot of server memory.

It is something we should fix, but it will probably require a chunk of time to investigate. Funding and pull requests are always welcome.

@bfriesenvcc

This comment has been minimized.

Copy link

commented Jan 30, 2019

OK, we only have 35 site-level role assignments, into 5 roles, so we're probably safe.

@thepurpleblob

This comment has been minimized.

Copy link

commented Apr 3, 2019

Me too!

@thepurpleblob

This comment has been minimized.

Copy link

commented Apr 3, 2019

Is $checkparentcontexts = true actually a requirement. Unless I'm completely missing something... surely we only care about users who are actually enrolled in this particular course?
It was hardly a comprehensive test but setting it to false doesn't make any difference in my experiments.

@danmarsden

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Sounds like that would make sense - It looks like it's checking the course context too. I wonder if we should be adding a check in the upgrade script to find any reengagements that return a different count of users when checking parentcontexts and notify the admin of the potential issue or if we should just change it....

@thepurpleblob

This comment has been minimized.

Copy link

commented Apr 4, 2019

I would normally say that any users who are involved with a course (which is pretty much 'by definition' for this plugin) should be enrolled in the course (and hence have a role in the course's context). Enrolling students (only) in the Course Category has always had undefined behaviour for many activities.

Having said that, there's bound to be a small number of users doing that. Personally, I would change the setting under the justification that it's a deprecated feature and just note it in the changelog or wherever.

@danmarsden danmarsden closed this in c13b8d3 Apr 5, 2019

@danmarsden

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I'm inclined to agree with you too! - I've just pushed a patch into master to do this - That list of participants page is still a bit funky - hard to understand what information it is actually providing - one day it might be nice to make it more useful.. I'll try to backport this to some of the older branches (and into the totara branches sometime soon.) thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.