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

Fix preferred log reader obtaining #221

Open
wants to merge 1 commit into
base: MOODLE_36_STABLE
Choose a base branch
from

Conversation

Royerino
Copy link

@Royerino Royerino commented Oct 20, 2022

There is a minor bug in the cr_logging_info() function in locallib.php. If more than one log reader is enabled on a Moodle site, it will ignore the order priority, as the foreach that iterates through the readers doesn't stop once it obtains the first reader. In my proposed solution, it will break the foreach once it obtains the first log reader established in the configuration, be it the standard log, the legacy log, or whichever other log reader is preferred.

This bug can be easily reproduced if you have some courses with user activity and try to create a configurable report that calculates the time spent on a course. If the preferred log reader is the Standard Log, it will show 0 hours for every user, as it won't find any record of user activity on the mdl_log, which is the one that is consulting due to this bug.

@sarjona
Copy link
Collaborator

sarjona commented Dec 1, 2022

Hi @Royerino!
Thanks for reporting this and working on a patch for fixing it. I've tried to reproduce the error but I haven't been able to... Could you please share some more detailed testing instructions? The first break looks OK to me but, as I don't know logs deeply, I'm worried because looking at the code it seems the idea is to prioritise logstore_legacy over sql_internal_table_reader or sql_internal_reader... so adding the second break might remove priority to logstore_legacy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants