I'd suggest we add a run-time check to prevent the infinite loop and throw an error. That's much better than potentially significantly degrade the performance. It will also make it easier to fix the problem and improve the user experience (error will be given instead of blank page or timeout).

Correct, we should make to code robust in the face of garbage data like this in the DB.

However, I am not sure your code goes far enough. Surely we can and should also detect longer loops like X is the parent category of Y, is the parent category of X.

Yeah, good point - we should be detecting loops no matter how long they are. This will this call a bit slower but it's probably worth doing it. I'll have a look later on.

OK, that should do - see the diff. As an added bonus I've removed recursion.

This is definitely going in the right direction.

You could make the code even more efficient: Use get_in_or_equal to process the whole of $templist each trip through the while loop.

Also, using get_records_sql_menu, or get_recordset_sql, would probably improve the efficiency too.

Picky points:

1. Code checker says that

// Comments should look like this.

//not like this

2. $templist is a bad variable name. It does not really say what this variable does.

I like the idea of limiting SQL queries, all done. It'll probably be easier to look at the full code than just a diff, here: https://github.com/enovation/moodle/blob/9f95b264e0bdfd618ce52336a1e23ec1f094eec9/lib/questionlib.php#L1179

Very nice! If you can just write some testing instructions, we can get this submitted for integration.

Great!

As a bug fix, please can you cherry-pick to stable branches after you have rebased, after this week's weeklies.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Rebased and cherry picked for 2.2 and 2.3 branches. Please review & integrate.

Thanks Tomasz and Tim, this has now been integrated.

Sorry Tomasz,

in all branches: I haven't received the exception. Instead the categories just disappear completely.

Jason: just to confirm you were testing with DEBUG_DEVELOPER?

$CFG->debug = 38911;
$CFG->xmlstrictheaders = false;
$CFG->debugdisplay = true;
$CFG->perfdebug = 15;
define('MDL_PERFDB' , true);
$CFG->upgradeshowsql = true;
$CFG->debugpageinfo = true;

That's my config script section for debugging, as per instructions from Sam and Aparup ... is that sufficient?

Right, I think that the problem is that this is very hard to test. I think the code is OK if you can work out how to test it.

As Jason observed, once you have a cycle of categories, they do not show up in the dropdown, menu, so you cannot choose them. So, to reproduce the original infinite, look, which now throws an exception, do this:

Suppose you start with three categories like

• id = 192 Default for Test course (parent = 0)
  • id = 200 Subcategory (parent = 192)
    • id = 201 Sub-sub-category (parent = 200)

Either:

1. Add a random question picking from "Default for Test course" and all subcategories to the quiz, or
2. Switch your question bank view so you are looking at category 192. (Make sure there is a bit like &category=192%2C109 in the URL.)

Then, directly in the database, try editing as follows:

1. Set the parent category of 192 to be 192, and reload the page. You should see the exception.
2. Set the parent category of 192 to be 201, and reload the page. You should see the exception.
3. Finally, you probably want to set the parent category of 192 back to 0.

Of course, your category ids will probably be different, but you get the idea.

Actually, I have just done all that, so why don't I assign myself as tester, and mark this as test passed?

Oh, I do not have permission to change the status of this issue, even if I make myself tester. Also, since I peer reviewed this, I probably should not be tester.

Back to the testing phase it goes

This is assigned to Tim, David has offered to test it so I'll re-assign it to him.

Tested in master and 22 following Tim comments (results explication with the same used IDs)

I've been able to see the exception changing 192 parent to be 201, 192 and also 200 with a categories hierarchy like the one used by Tim:
Default for COURSENAME (192 in Tim example)

Then I added more categories to the structure to finish with something like this:
Default for COURSENAME (192 in Tim example)

With this scenario when changing the 192 parent to 201 I can see the exception, but not when changing 192 parent to 192 nor 200, the categories doesn't appear and the dropdown menu focuses on 'Default for COURSECATEGORYNAME' (with URL like &category=192%2C50) but there is no exception.

No infinite loop in any case

Closing as fixed, many thanks for your awesome collaboration.