Fix silent failures in checkExistingCategory#24
Open
giovanny07 wants to merge 1 commit into
Open
Conversation
- Target entity was not added to the visible entities list when ancestors_cache was empty (root entity or unpopulated cache after migration), causing categories that belong directly to the target to be incorrectly reported as non-existent. - Added early return when the ticket has no category (id 0) to avoid unnecessary queries and loose-comparison pitfalls. - Added early return when the category row is missing from the DB. - Replaced empty-string defaults and loose == comparisons with proper int/bool types and strict === checks to prevent PHP type coercion issues (e.g. '' == 0 evaluating to true). - Simplified the recursion/ancestor logic: a category that belongs directly to the target entity is always visible; otherwise it must be recursive and belong to an ancestor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ancestors_cacheisempty (root entity or unpopulated cache after a DB migration/entity move), the
target entity itself was never added to the ancestors array. Categories belonging
directly to the target entity were silently reported as non-existent, causing them
to be reset to empty on transfer.
When
itilcategories_id = 0, the function queriedglpi_itilcategories WHERE id = 0(no results), leaving variables as empty strings. PHP loose comparison
'' == 0evaluates to
true, which could produce incorrect results depending on the targetentity.
glpi_itilcategories, the variables stayed as empty strings and the function reliedon loose comparisons to reach a result instead of returning
falseimmediately.==throughout: The original code used''asthe default for entity IDs and booleans, then compared them with
==and default(loose)
in_array. PHP type coercion ('' == 0→true,in_array('', [0])→true) made the results unpredictable in edge cases.Changes
src/Ticket.php—checkExistingCategory():$targetEntityto the visible entities list, regardless of whetherancestors_cacheis populated.falsewhen the ticket has no category (itilcategories_id = 0).falsewhen the category does not exist inglpi_itilcategories.intand flags tobool; use strict===andin_array(..., true).is always visible; otherwise it must be recursive and belong to an ancestor.
Test plan
— category must be preserved
recursive — category must be preserved
— category must be reset
itilcategories_id = 0) — no errors, categorystays empty
glpi_itilcategories— categorymust be reset without errors