Skip to content

User: skip broken users in gallery (47791) #11638

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.

Sign up for GitHub

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

Open
schmitz-ilias wants to merge 2 commits into
base: release_10
Choose a base branch
from
Open

User: skip broken users in gallery (47791) #11638

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b3cc07
User: skip broken users in gallery (47791)
schmitz-ilias Jun 2, 2026
fd02461
fix cp notice
schmitz-ilias Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions components/ILIAS/User/classes/Gallery/class.ilUsersGalleryParticipants.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
<?php

declare(strict_types=1);

/**
* This file is part of ILIAS, a powerful learning management system
* published by ILIAS open source e-Learning e.V.
Expand All @@ -18,6 +16,8 @@
*
*********************************************************************/

declare(strict_types=1);

/**
* Class ilUsersGalleryParticipants
*/
Expand All @@ -43,6 +43,10 @@ protected function getUsers(array $usr_ids): array
continue;
}

if (!ilObjUser::userExists([$usr_id])) {
Copy link
Copy Markdown
Contributor

@mjansenDatabay mjansenDatabay Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really do this? This will lead to N additional SQL queries being fired.

Copy link
Copy Markdown
Contributor Author

@schmitz-ilias schmitz-ilias Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we can catch this any other way than with additional queries. As far as I can tell, the first time usr_data is read out in the call from the ticket is in the constructor of ilObjUser, immediately before the error occurs. As @kergomard said in the ticket, there is really no good way to get out of the failed instantiation of the user once we get to that point.

It would of course be possible to do the check with a single additional query. User would have to offer some kind of filterOutBrokenUsers utility method, so that we are able to do the check at scale. Not sure where that would go, adding another static method to ilObjUser seems ill advised.

I guess a different fix here would involve (periodically?) cleaning up broken users before they become a problem. Or did you have something else in mind?

Copy link
Copy Markdown
Contributor

@mjansenDatabay mjansenDatabay Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tim,

Thanks for the detailed explanation. I think your assessment is correct: within the current architecture there is probably no easy way to avoid the additional queries once we instantiate ilObjUser, because the consistency issue is only detected during object construction.

Personally, I find the idea of periodically checking and cleaning up broken records quite compelling. Since I do not expect ILIAS to adopt (component-spanning) database-level referential integrity constraints in the foreseeable future (the topic has been discussed multiple times over the years, including workshops and proposals documented in the "Feature Wiki"), I think periodic and manually triggerable integrity checks are probably the most pragmatic long-term solution.

In fact, I could imagine extending the "System Check" infrastructure in a more generic way. Instead of relying on hard-coded task definitions, components could contribute integrity checks and optional cleanup tasks through artifacts or future component integration mechanisms. A generic "Integrity Cleanup" framework could then detect and optionally repair/delete orphaned or otherwise inconsistent records across the system. Besides improving data integrity, this would likely also have a positive impact on the performance of long-running installations.

Regarding alternatives, I can think of a few options:

  • As you also mentionend: Providing an API that can identify broken user records (or their IDs) with a single query. Or: Providing a utility that filters a given set of user IDs and removes invalid entries in a scalable way.
  • Replacing ilErrorHandler::raiseError(..., FATAL) in ilObjUser with regular exceptions that consumers can catch and handle gracefully.
  • Avoiding ilObjUser altogether in this context and introducing a dedicated gallery-facing entity (e.g. GalleryProfile) that exposes only the data required by the "Gallery" domain and can be loaded efficiently from a dedicated repository (ensuring the integrity of these provided entities).

That said, I fully agree that most of these ideas go well beyond the scope of a bug fix. I mainly wanted to point them out because performance and data consistency issues tend to become increasingly visible in larger and older installations.
In the hosting environments I am involved with (passively), we have observed measurable performance regressions when comparing ILIAS 8, 9 and 10 under otherwise comparable usage scenarios and data volumes. While the reasons are often multifaceted and not attributable to a single change, the overall trend is noticeable.
Beyond my own observations, similar concerns are raised within the "SIG ILIAS betreiben", suggesting that this is not merely an isolated experience. Changes that introduce additional database queries therefore tend to attract my attention.

Best regards,
Michael

continue;
}

if (!($user = ilObjectFactory::getInstanceByObjId($usr_id, false)) || !($user instanceof ilObjUser)) {
continue;
}
Expand Down
Loading