Skip to content

Fix silent failures in checkExistingCategory#24

Open
giovanny07 wants to merge 1 commit into
pluginsGLPI:masterfrom
giovanny07:fix/check-existing-category
Open

Fix silent failures in checkExistingCategory#24
giovanny07 wants to merge 1 commit into
pluginsGLPI:masterfrom
giovanny07:fix/check-existing-category

Conversation

@giovanny07

Copy link
Copy Markdown

Summary

  • Target entity missing from visible entities list: When ancestors_cache is
    empty (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.
  • Unnecessary queries and loose-comparison pitfalls for tickets with no category:
    When itilcategories_id = 0, the function queried glpi_itilcategories WHERE id = 0
    (no results), leaving variables as empty strings. PHP loose comparison '' == 0
    evaluates to true, which could produce incorrect results depending on the target
    entity.
  • No guard for deleted categories: If the category row no longer exists in
    glpi_itilcategories, the variables stayed as empty strings and the function relied
    on loose comparisons to reach a result instead of returning false immediately.
  • Empty-string defaults and loose == throughout: The original code used '' as
    the default for entity IDs and booleans, then compared them with == and default
    (loose) in_array. PHP type coercion ('' == 0true, in_array('', [0])
    true) made the results unpredictable in edge cases.

Changes

src/Ticket.phpcheckExistingCategory():

  1. Always add $targetEntity to the visible entities list, regardless of whether
    ancestors_cache is populated.
  2. Early return false when the ticket has no category (itilcategories_id = 0).
  3. Early return false when the category does not exist in glpi_itilcategories.
  4. Cast all IDs to int and flags to bool; use strict === and in_array(..., true).
  5. Simplified visibility logic: a category that belongs directly to the target entity
    is always visible; otherwise it must be recursive and belong to an ancestor.

Test plan

  • Transfer a ticket whose category belongs directly to the target entity (non-recursive)
    — category must be preserved
  • Transfer a ticket whose category belongs to an ancestor of the target entity and is
    recursive — category must be preserved
  • Transfer a ticket whose category belongs to an ancestor but is not recursive
    — category must be reset
  • Transfer a ticket with no category (itilcategories_id = 0) — no errors, category
    stays empty
  • Transfer a ticket whose category was deleted from glpi_itilcategories — category
    must be reset without errors
  • Transfer to the root entity (entity 0) — verify correct behavior in all cases above

- 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.
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.

1 participant