N°9687 - Fix incorrect return type for the DBObjectSearch::ApplyDataFilter() function by Molkobain · Pull Request #939 · Combodo/iTop

Molkobain · 2026-06-17T08:12:36Z

Base information

Question	Answer
Related to a SourceForge thread / Another PR / A GitHub Issue / Combodo ticket?	N°9687
Type of change?	Bug fix

Symptom (bug)

Fatal error when accessing the backoffice with a non-admin user.

2026-06-15 09:54:55 | Error   | 3     | Uncaught TypeError: DBObjectSearch::ApplyDataFilters(): Return value must be of type DBObjectSearch, DBUnionSearch returned in /var/www/php8.1/pro322-1/core/dbobjectsearch.class.php:1960
Stack trace:
#0 /var/www/php8.1/pro322-1/core/dbsearch.class.php(1125): DBObjectSearch->ApplyDataFilters()
#1 /var/www/php8.1/pro322-1/core/dbsearch.class.php(1039): DBSearch->GetSQLQuery()
#2 /var/www/php8.1/pro322-1/core/metamodel.class.php(6181): DBSearch->MakeSelectQuery()
#3 /var/www/php8.1/pro322-1/core/metamodel.class.php(6368): MetaModel::MakeSingleRow()
#4 /var/www/php8.1/pro322-1/core/metamodel.class.php(6281): MetaModel::GetObjectWithArchive()
#5 /var/www/php8.1/pro322-1/core/userrights.class.inc.php(334): MetaModel::GetObject()
#6 /var/www/php8.1/pro322-1/core/userrights.class.inc.php(1336): User->GetContactObject()
#7 /var/www/php8.1/pro322-1/core/userrights.class.inc.php(1280): UserRights::GetContactObject()
#8 /var/www/php8.1/pro322-1/sources/Application/UI/Base/Layout/NavigationMenu/NavigationMenu.php(476): UserRights::GetContactOrganizationFriendlyname()
#9 /var/www/php8.1/pro322-1/sources/Application/UI/Base/Layout/NavigationMenu/NavigationMenu.php(130): Combodo\iTop\Application\UI\Base\Layout\NavigationMenu\NavigationMenu->ComputeUserData()
#10 /var/www/php8.1/pro322-1/sources/Application/UI/Base/Layout/NavigationMenu/NavigationMenuFactory.php(59): Combodo\iTop\Application\UI\Base\Layout\NavigationMenu\NavigationMenu->__construct()
#11 /var/www/php8.1/pro322-1/sources/Application/WebPage/iTopWebPage.php(149): Combodo\iTop\Application\UI\Base\Layout\NavigationMenu\NavigationMenuFactory::MakeStandard()
#12 /var/www/php8.1/pro322-1/pages/UI.php(267): Combodo\iTop\Application\WebPage\iTopWebPage->__construct()
#13 {main}
 thrown | IssueLog |||
array (
 'type' => 1,
 'file' => '/var/www/php8.1/pro322-1/core/dbobjectsearch.class.php',
 'line' => 1960,
)

Reproduction procedure (bug)

On iTop 3.2.3-1
The XML delta below
A user with Support agent profile and allowed organizations
Log in the backoffice
Try to create a ticket

Cause (bug)

Fix a fatal type mismatch in data filtering when user rights filters return a union search.

DBObjectSearch::ApplyDataFilters() is currently typed to return DBObjectSearch, but in some valid scenarios (custom rights profile / team silos), filters return DBUnionSearch.
This causes a runtime TypeError and breaks login, impersonation, and some non-admin actions (for example ticket creation).

Proposed solution (bug and enhancement)

The fix is to broaden the return type from DBObjectSearch to DBSearch, which matches actual behavior and accepted filter outputs.

Checklist before requesting a review

I have performed a self-review of my code
I have tested all changes I made on an iTop instance
I have added a unit test, otherwise I have explained why I couldn't
Is the PR clear and detailed enough so anyone can understand without digging in the code?

…ilter() function

Molkobain · 2026-06-17T08:13:18Z

Draft until unit tests are pushed

Molkobain · 2026-06-17T08:13:52Z

@greptileai review please

greptile-apps · 2026-06-17T08:16:48Z

Greptile Summary

This PR broadens data-filter search return types to handle union filters. The main changes are:

DBObjectSearch::ApplyDataFilters() now returns the base DBSearch type.
DBUnionSearch::ApplyDataFilters() now matches the same base return type.
A regression test covers a user-rights filter returning DBUnionSearch.
The test fixture provides a custom GetSelectFilter() implementation for the union-filter case.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
core/dbobjectsearch.class.php	Return type now matches the existing `DBSearch` abstraction used by callers.
core/dbunionsearch.class.php	Return type now stays compatible with recursive filter application.
tests/php-unit-tests/unitary-tests/core/DBSearch/DBSearchApplyDataFiltersTest.php	Regression coverage exercises a user-rights filter that returns a union search and restores the selected module afterward.

_{Reviews (4): Last reviewed commit: "N°9687 - Improve unit test after code re..." | Re-trigger Greptile}

Copilot

Pull request overview

Fixes a fatal PHP TypeError caused by DBObjectSearch::ApplyDataFilters() being declared to return DBObjectSearch while it can legitimately return a DBUnionSearch (eg. when user rights filters produce union searches). The change aligns method signatures with actual runtime behavior by using the common base type DBSearch.

Changes:

Broadened ApplyDataFilters() return type in DBObjectSearch from DBObjectSearch to DBSearch to prevent runtime return type violations.
Broadened ApplyDataFilters() return type in DBUnionSearch from DBUnionSearch to DBSearch for consistency and to accommodate parent::ApplyDataFilters() behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
core/dbobjectsearch.class.php	Updates `ApplyDataFilters()` return type to `DBSearch` to avoid `TypeError` when filtering returns a `DBUnionSearch`.
core/dbunionsearch.class.php	Updates `ApplyDataFilters()` return type to `DBSearch`, matching the base contract and supporting delegation to `parent::ApplyDataFilters()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

odain-cbd · 2026-06-18T13:04:28Z

-	protected function ApplyDataFilters(): DBObjectSearch
+	protected function ApplyDataFilters(): DBSearch
 	{
 		if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {


IsDataFiltered() is doing the opposite of what the name seems to tell... hard to understand at first sight.

I strongly advice to complete PHPDOC of all used methods... some are missing or incomplete (mixed).

ie GetSearches, FromEmptySet, GetSelectedClasses, GetJoinedClasses.

apart from that if we do the minimum to change the code, it could be nice to have full test cover of 3 distincts kinds of ApplyDataFilters (DBUnionSearch, DBsearch, DBObjSearch().

for example security.disable_joined_classes_filter in the conf can change the behaviour. just as UserRightsGetSelectFilter extensibility.... we can discuss it if you want

last but not least it could be nice to refactor in one common method. where we would pass an array of string (coming from either GetSearches, GetSelectedClasses or GetJoinedClasses).

// Apply filter (this is similar to the one in DBSearch but the factorization could make it less readable) foreach ($aClassesToFilter as $sClassAlias => $sClass) { $oVisibleObjects = UserRights::GetSelectFilter($sClass, $this->GetModifierProperties('UserRightsGetSelectFilter')); if ($oVisibleObjects === false) { $oVisibleObjects = DBObjectSearch::FromEmptySet($sClass); } if (is_object($oVisibleObjects)) { $oVisibleObjects->AllowAllData(); $oSearch = $oSearch->Filter($sClassAlias, $oVisibleObjects); $oSearch->SetDataFiltered(); } }

no need to compute GetSelectedClass if 'security.disable_joined_classes_filter' is disabled.

odain-cbd

we should discuss... my guess is that we will focus on the minimum. which is not 100% sure from what i see in the code. i understand that i dont understand everything....

odain-cbd · 2026-06-18T13:06:39Z

-	protected function ApplyDataFilters(): DBObjectSearch
+	protected function ApplyDataFilters(): DBSearch
 	{
 		if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {


I strongly advice to complete PHPDOC of all used methods... some are missing or incomplete (mixed).

ie GetSearches, FromEmptySet, GetSelectedClasses, GetJoinedClasses.

odain-cbd · 2026-06-18T13:09:00Z

-	protected function ApplyDataFilters(): DBObjectSearch
+	protected function ApplyDataFilters(): DBSearch
 	{
 		if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {


apart from that if we do the minimum to change the code, it could be nice to have full test cover of 3 distincts kinds of ApplyDataFilters (DBUnionSearch, DBsearch, DBObjSearch().

for example security.disable_joined_classes_filter in the conf can change the behaviour. just as UserRightsGetSelectFilter extensibility.... we can discuss it if you want

odain-cbd · 2026-06-18T13:10:39Z

-	protected function ApplyDataFilters(): DBObjectSearch
+	protected function ApplyDataFilters(): DBSearch
 	{
 		if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {


last but not least it could be nice to refactor in one common method. where we would pass an array of string (coming from either GetSearches, GetSelectedClasses or GetJoinedClasses).

// Apply filter (this is similar to the one in DBSearch but the factorization could make it less readable) foreach ($aClassesToFilter as $sClassAlias => $sClass) { $oVisibleObjects = UserRights::GetSelectFilter($sClass, $this->GetModifierProperties('UserRightsGetSelectFilter')); if ($oVisibleObjects === false) { $oVisibleObjects = DBObjectSearch::FromEmptySet($sClass); } if (is_object($oVisibleObjects)) { $oVisibleObjects->AllowAllData(); $oSearch = $oSearch->Filter($sClassAlias, $oVisibleObjects); $oSearch->SetDataFiltered(); } }

odain-cbd · 2026-06-18T13:11:20Z

-	protected function ApplyDataFilters(): DBObjectSearch
+	protected function ApplyDataFilters(): DBSearch
 	{
 		if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {


no need to compute GetSelectedClass if 'security.disable_joined_classes_filter' is disabled.

odain-cbd · 2026-06-18T13:13:07Z

-	 * @return DBUnionSearch
 	 */
-	protected function ApplyDataFilters(): DBUnionSearch
+	protected function ApplyDataFilters(): DBSearch


it is a pity that we compute further times IsDataAllowed and IsDataFiltered when dealing with Union....

first time in DBUnion. and 2nd time in the loop when calling each sub item.

Molkobain · 2026-06-18T17:47:37Z

@greptileai review again please.

Molkobain · 2026-06-18T17:50:01Z

we should discuss... my guess is that we will focus on the minimum. which is not 100% sure from what i see in the code. i understand that i dont understand everything....

Thanks for the review. We can discuss IRL tomorrow for sure.
Keep in mind that this PR addresses a blocking issue on iTop 3.2.3-2 and is for the 3.2.3-2 micro version, we just want to fix the bug, not rework the feature. But let's dig into this IRL :)

Molkobain · 2026-06-18T18:07:24Z

@greptileai review again please.

N°9687 - Fix incorrect return type for the DBObjectSearch::ApplyDataF…

e1d274b

…ilter() function

Molkobain self-assigned this Jun 17, 2026

Molkobain added the bug Something isn't working label Jun 17, 2026

Molkobain added this to the 3.2.3-2 milestone Jun 17, 2026

Molkobain requested a review from Copilot June 17, 2026 08:21

Copilot started reviewing on behalf of Molkobain June 17, 2026 08:22 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

N°9687 - Add unit test

0f7653a

Molkobain requested review from accognet, eespie, odain-cbd and steffunky June 18, 2026 10:02

Molkobain marked this pull request as ready for review June 18, 2026 10:02

greptile-apps Bot reviewed Jun 18, 2026

View reviewed changes

Comment thread tests/php-unit-tests/unitary-tests/core/DBSearch/DBSearchApplyDataFiltersTest.php

odain-cbd reviewed Jun 18, 2026

View reviewed changes

odain-cbd requested changes Jun 18, 2026

View reviewed changes

N°9687 - Improve unit test after code review

2466406

greptile-apps Bot reviewed Jun 18, 2026

View reviewed changes

Comment thread tests/php-unit-tests/unitary-tests/core/DBSearch/DBSearchApplyDataFiltersTest.php

N°9687 - Improve unit test after code review

14d1862

Conversation

Molkobain commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Base information

Symptom (bug)

Reproduction procedure (bug)

Cause (bug)

Proposed solution (bug and enhancement)

Checklist before requesting a review

Uh oh!

Molkobain commented Jun 17, 2026

Uh oh!

Molkobain commented Jun 17, 2026

Uh oh!

greptile-apps Bot commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odain-cbd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Molkobain commented Jun 18, 2026

Uh oh!

Uh oh!

Molkobain commented Jun 18, 2026

Uh oh!

Molkobain commented Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Molkobain commented Jun 17, 2026 •

edited

Loading

greptile-apps Bot commented Jun 17, 2026 •

edited

Loading