{{ message }}
Fix/attribute search refactor#4442
Open
jekkos wants to merge 11 commits into
Open
Conversation
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Item.php`:
- Around line 152-155: The attribute search block in Item.php currently only
checks attribute_values.attribute_value and attribute_values.attribute_decimal,
so date-type attributes are ignored; update the query builder inside the group
(the lines using $builder->like('attribute_values.attribute_value', $search) and
$builder->orWhere('attribute_values.attribute_decimal', $search)) to also
include attribute_values.attribute_date (e.g. add an
$builder->orWhere('attribute_values.attribute_date', $search) or an appropriate
date-normalized comparison) so DATE-type attributes are matched—ensure you
normalize/format $search to the stored date format if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cd03a10-85af-4204-b7a0-e1341a6a69a8
📒 Files selected for processing (6)
app/Controllers/Attributes.phpapp/Controllers/Items.phpapp/Language/en-GB/Attributes.phpapp/Language/en/Attributes.phpapp/Models/Attribute.phpapp/Models/Item.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Item.php (1)
393-393:⚠️ Potential issue | 🟠 MajorAvoid direct interpolation of
definition_idsinto SQL fragments.Building
IN (...)via rawimplode()in model SQL is risky and brittle; sanitize/cast IDs before composing the join fragment.As per coding guidelines,
{app/Models/**/*.php,app/Database/Migrations/**/*.php}: Use parameterized queries to prevent SQL injection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` at line 393, The join fragment in Item.php directly interpolates filters['definition_ids'] into SQL via implode, which is unsafe; update the code that builds the join ($builder->join(...)) to sanitize/cast each value in filters['definition_ids'] to integers and use parameter binding or the query builder's whereIn/compose-join-with-placeholders approach instead of raw string interpolation (target the $builder->join call referencing attribute_links and definition_id), ensuring all IDs are validated (e.g., array_map('intval', ...)) before passing them as bound parameters.
♻️ Duplicate comments (2)
app/Models/Item.php (2)
255-257:⚠️ Potential issue | 🟠 MajorDate attributes are still excluded from attribute value search.
This path searches text/decimal only, so DATE definitions remain non-searchable through generic attribute-value matching.
Proposed fix
$builder->groupStart(); $builder->like('attribute_values.attribute_value', $search); $builder->orWhere('attribute_values.attribute_decimal', $search); + $builder->orWhere('attribute_values.attribute_date', $search); $builder->groupEnd();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 255 - 257, The attribute-value search group currently checks only attribute_value (text) and attribute_decimal, leaving DATE attributes out; update the search block in Item.php to include attribute_values.attribute_date in the same group (e.g., add an orWhere for attribute_values.attribute_date) and ensure the search value is normalized for date comparison (parse/format the input into a YYYY-MM-DD or use a DB cast) so date-typed attributes are matched by the generic attribute-value search in the same method where $builder->like('attribute_values.attribute_value', $search) and $builder->orWhere('attribute_values.attribute_decimal', $search) are used.
183-184:⚠️ Potential issue | 🟡 MinorUse strict emptiness checks for search inputs.
empty($search)treats"0"as empty, so valid numeric/checkbox attribute searches can be skipped.Proposed fix
- if (empty($definition_ids) || empty($search)) { + if ($definition_ids === [] || $search === '') { return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 183 - 184, The current check uses empty($search) which treats the string "0" as empty and will incorrectly skip valid searches; update the condition around $definition_ids/$search to only treat null/empty-string (and truly empty arrays) as empty — e.g. replace empty($search) with ($search === null || $search === '' || (is_array($search) && empty($search))) so numeric/checkbox values like "0" are preserved; adjust the if condition that references $definition_ids and $search in the Item model accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/Items.php`:
- Around line 111-113: Remove the redundant call and unused local: eliminate the
unused $attribute_column_ids variable created from array_keys($definition_names)
and remove the repeated call to
$this->attribute->get_definitions_by_flags(Attribute::SHOW_IN_ITEMS) later in
the method; instead reuse the initial $definition_names value (from the first
get_definitions_by_flags call) wherever definitions are needed to avoid
duplicate queries and dead locals.
In `@app/Models/Item.php`:
- Around line 192-197: The loop assumes $all_definitions entries are arrays with
['name'], but get_definitions_by_flags(Attribute::SHOW_IN_ITEMS |
Attribute::SHOW_IN_SEARCH) returns definition_id => definition_name (string)
unless types are requested, causing offset-on-string errors; update the code
around $all_definitions and the foreach that builds $definition_name_to_id to
handle both shapes — detect if $def_info is a string and use it as the name,
otherwise read $def_info['name'], then store strtolower($name) => $id;
alternatively, call get_definitions_by_flags with the flag that returns typed
defs and adapt accordingly (adjust code in the loop and the call site where
$all_definitions is created).
- Around line 232-235: The code currently returns term-only matches when
$parsed['terms'] exists, discarding previously accumulated attribute matches;
instead of returning in that block, call $this->search_by_attribute_value($term,
$definition_ids, $include_deleted) and merge its results with the existing
$definition_ids (e.g. array_merge + array_unique or the equivalent) so
attribute-matched IDs are preserved, then continue the method flow using the
updated $definition_ids; reference symbols: $parsed['terms'], $term,
search_by_attribute_value, $definition_ids, $include_deleted.
---
Outside diff comments:
In `@app/Models/Item.php`:
- Line 393: The join fragment in Item.php directly interpolates
filters['definition_ids'] into SQL via implode, which is unsafe; update the code
that builds the join ($builder->join(...)) to sanitize/cast each value in
filters['definition_ids'] to integers and use parameter binding or the query
builder's whereIn/compose-join-with-placeholders approach instead of raw string
interpolation (target the $builder->join call referencing attribute_links and
definition_id), ensuring all IDs are validated (e.g., array_map('intval', ...))
before passing them as bound parameters.
---
Duplicate comments:
In `@app/Models/Item.php`:
- Around line 255-257: The attribute-value search group currently checks only
attribute_value (text) and attribute_decimal, leaving DATE attributes out;
update the search block in Item.php to include attribute_values.attribute_date
in the same group (e.g., add an orWhere for attribute_values.attribute_date) and
ensure the search value is normalized for date comparison (parse/format the
input into a YYYY-MM-DD or use a DB cast) so date-typed attributes are matched
by the generic attribute-value search in the same method where
$builder->like('attribute_values.attribute_value', $search) and
$builder->orWhere('attribute_values.attribute_decimal', $search) are used.
- Around line 183-184: The current check uses empty($search) which treats the
string "0" as empty and will incorrectly skip valid searches; update the
condition around $definition_ids/$search to only treat null/empty-string (and
truly empty arrays) as empty — e.g. replace empty($search) with ($search ===
null || $search === '' || (is_array($search) && empty($search))) so
numeric/checkbox values like "0" are preserved; adjust the if condition that
references $definition_ids and $search in the Item model accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4b18c3d-0631-4f4f-b087-881783a8e7a1
📒 Files selected for processing (3)
app/Controllers/Items.phpapp/Helpers/tabular_helper.phpapp/Models/Item.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/Models/Item.php (3)
191-196:⚠️ Potential issue | 🔴 CriticalFix definition payload shape before building
$definition_name_to_id.Line 191 calls
get_definitions_by_flags()without typed payload, but Line 195 reads$def_info['name']. With default return shape, this can trigger a runtime error (Cannot access offset of type string on string).🔧 Proposed fix
- $all_definitions = $attribute->get_definitions_by_flags(Attribute::SHOW_IN_ITEMS | Attribute::SHOW_IN_SEARCH); + $all_definitions = $attribute->get_definitions_by_flags( + Attribute::SHOW_IN_ITEMS | Attribute::SHOW_IN_SEARCH, + true + ); $definition_name_to_id = []; foreach ($all_definitions as $id => $def_info) { - $definition_name_to_id[strtolower($def_info['name'])] = $id; + $definition_name_to_id[strtolower($def_info['name'])] = (int) $id; }#!/bin/bash # Verify return shape + current call/usage mismatch rg -n "function get_definitions_by_flags|include_types = false|return \\$this->to_array" app/Models/Attribute.php -C2 rg -n "get_definitions_by_flags\\(Attribute::SHOW_IN_ITEMS \\| Attribute::SHOW_IN_SEARCH" app/Models/Item.php -C2 rg -n "\\$def_info\\['name'\\]" app/Models/Item.php -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 191 - 196, The code calls Attribute::get_definitions_by_flags(...) and immediately treats each $def_info as an array with a 'name' key when building $definition_name_to_id, but get_definitions_by_flags can return a string/default shape; update the call to request the typed/array payload (e.g., enable the include_types/array return option) or transform the returned values into the expected array shape before the foreach so $def_info['name'] is always valid; specifically modify the get_definitions_by_flags invocation used to populate $all_definitions and/or add a normalization step prior to the foreach that ensures each $def_info contains a 'name' key (referencing get_definitions_by_flags, $all_definitions, $definition_name_to_id and $def_info['name']).
253-256:⚠️ Potential issue | 🟠 MajorInclude DATE attributes in value-based attribute search.
This search path currently checks text and decimal only; DATE attributes remain unsearchable.
🔧 Proposed fix
$builder->groupStart(); $builder->like('attribute_values.attribute_value', $search); $builder->orWhere('attribute_values.attribute_decimal', $search); + $builder->orWhere('attribute_values.attribute_date', $search); $builder->groupEnd();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 253 - 256, The attribute-value search currently only checks attribute_values.attribute_value and attribute_values.attribute_decimal; update the query inside the Item model where $builder->groupStart()/groupEnd() is used to also include the date column by adding a condition for attribute_values.attribute_date (e.g., an orWhere or a date-aware match) so DATE attributes are matched by the same search flow; locate the block that calls $builder->like('attribute_values.attribute_value', $search) and $builder->orWhere('attribute_values.attribute_decimal', $search) and add an appropriate $builder->orWhere(...) for attribute_values.attribute_date (using a date comparison or formatted string match consistent with other searches).
231-234:⚠️ Potential issue | 🟠 MajorDo not drop attribute-matched IDs when free-text terms exist.
Line 233 returns term-only results and discards IDs already found from attribute clauses, breaking mixed queries.
🔧 Proposed fix
if (!empty($parsed['terms'])) { $term = implode(' ', $parsed['terms']); - return $this->search_by_attribute_value($term, $definition_ids, $include_deleted); + $term_item_ids = $this->search_by_attribute_value($term, $definition_ids, $include_deleted); + + if (empty($matching_item_ids)) { + return $term_item_ids; + } + + return $logic === 'AND' + ? array_values(array_intersect($matching_item_ids, $term_item_ids)) + : array_values(array_unique(array_merge($matching_item_ids, $term_item_ids))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 231 - 234, When free-text terms exist we must not return only term-only results and drop IDs already found from attribute clauses; instead call search_by_attribute_value($term, $definition_ids, $include_deleted) to get term-matched IDs, then merge those with the attribute-matched ID array produced earlier (the array used for attribute clause results), deduplicate (unique) and return the combined ID set while preserving $include_deleted and $definition_ids behavior; update the early return in the parsed['terms'] branch so it combines results rather than replacing them (use the existing attribute-matched IDs variable defined earlier).
🧹 Nitpick comments (1)
app/Models/Item.php (1)
140-271: Prefer camelCase naming for newly added PHP methods.New method names (
parse_attribute_search,search_by_attributes,search_by_attribute_value,get_attribute_sort_definition_id) and related locals are snake_case; this diverges from the repository naming rule for changed PHP code.As per coding guidelines, "Use PSR-12 naming conventions:
camelCasefor variables and functions,PascalCasefor classes,UPPER_CASEfor constants."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 140 - 271, Rename the new snake_case methods and related locals to camelCase and update all their internal and external references: parse_attribute_search -> parseAttributeSearch, search_by_attributes -> searchByAttributes, search_by_attribute_value -> searchByAttributeValue, get_attribute_sort_definition_id -> getAttributeSortDefinitionId; also convert local variable names (e.g., $definition_ids -> $definitionIds, $include_deleted -> $includeDeleted, $matching_item_ids -> $matchingItemIds, $definition_name_to_id -> $definitionNameToId, $attr_name/$attrValue etc.) and update every call site in this class (and any other files) to use the new names so signatures and usages remain consistent with PSR-12 camelCase rules. Ensure method visibility and PHPDoc remain correct and run tests/static analysis to catch missed references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Item.php`:
- Around line 206-211: The attribute search only compares
attribute_values.attribute_value (text) so numeric/date attributes are missed;
update the query that builds $builder (joining attribute_links and
attribute_values in Item model) to search across the typed value columns too
(e.g. attribute_values.attribute_value_decimal and
attribute_values.attribute_value_date or whatever your schema uses) by adding an
ORed set of comparisons (use $builder->groupStart()/orLike()/orWhere() or
equivalent to combine attribute_value, attribute_value_decimal and
attribute_value_date checks) while keeping the existing
where('attribute_links.definition_id', $definition_id) filter.
---
Duplicate comments:
In `@app/Models/Item.php`:
- Around line 191-196: The code calls Attribute::get_definitions_by_flags(...)
and immediately treats each $def_info as an array with a 'name' key when
building $definition_name_to_id, but get_definitions_by_flags can return a
string/default shape; update the call to request the typed/array payload (e.g.,
enable the include_types/array return option) or transform the returned values
into the expected array shape before the foreach so $def_info['name'] is always
valid; specifically modify the get_definitions_by_flags invocation used to
populate $all_definitions and/or add a normalization step prior to the foreach
that ensures each $def_info contains a 'name' key (referencing
get_definitions_by_flags, $all_definitions, $definition_name_to_id and
$def_info['name']).
- Around line 253-256: The attribute-value search currently only checks
attribute_values.attribute_value and attribute_values.attribute_decimal; update
the query inside the Item model where $builder->groupStart()/groupEnd() is used
to also include the date column by adding a condition for
attribute_values.attribute_date (e.g., an orWhere or a date-aware match) so DATE
attributes are matched by the same search flow; locate the block that calls
$builder->like('attribute_values.attribute_value', $search) and
$builder->orWhere('attribute_values.attribute_decimal', $search) and add an
appropriate $builder->orWhere(...) for attribute_values.attribute_date (using a
date comparison or formatted string match consistent with other searches).
- Around line 231-234: When free-text terms exist we must not return only
term-only results and drop IDs already found from attribute clauses; instead
call search_by_attribute_value($term, $definition_ids, $include_deleted) to get
term-matched IDs, then merge those with the attribute-matched ID array produced
earlier (the array used for attribute clause results), deduplicate (unique) and
return the combined ID set while preserving $include_deleted and $definition_ids
behavior; update the early return in the parsed['terms'] branch so it combines
results rather than replacing them (use the existing attribute-matched IDs
variable defined earlier).
---
Nitpick comments:
In `@app/Models/Item.php`:
- Around line 140-271: Rename the new snake_case methods and related locals to
camelCase and update all their internal and external references:
parse_attribute_search -> parseAttributeSearch, search_by_attributes ->
searchByAttributes, search_by_attribute_value -> searchByAttributeValue,
get_attribute_sort_definition_id -> getAttributeSortDefinitionId; also convert
local variable names (e.g., $definition_ids -> $definitionIds, $include_deleted
-> $includeDeleted, $matching_item_ids -> $matchingItemIds,
$definition_name_to_id -> $definitionNameToId, $attr_name/$attrValue etc.) and
update every call site in this class (and any other files) to use the new names
so signatures and usages remain consistent with PSR-12 camelCase rules. Ensure
method visibility and PHPDoc remain correct and run tests/static analysis to
catch missed references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff45576-1fed-4086-80f4-cda32a99999a
📒 Files selected for processing (3)
app/Controllers/Items.phpapp/Helpers/tabular_helper.phpapp/Models/Item.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Helpers/tabular_helper.php
objecttothis
requested changes
Mar 17, 2026
objecttothis
left a comment
objecttothis
left a comment
Member
There was a problem hiding this comment.
Mostly just issues with new code with variables which are not PSR-12 compliant. Then also the one discussion about using : as a delimeter.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/Models/Item.php (1)
151-159:⚠️ Potential issue | 🟠 MajorSupport multi-word attribute names in the parser.
The current key pattern (
\w+) drops definitions likeaspect ratio:16:9(it capturesratio, notaspect ratio), so valid attribute-qualified searches are missed.🔧 Proposed fix
- $pattern = '/(\w+)\s*:\s*([^\s,]+)(?:\s+(?:AND|OR)\s+)?/i'; + // Supports keys with spaces/hyphens and values containing ":" (e.g. 16:9) + $pattern = '/([[:alnum:]_][[:alnum:] _-]*?)\s*:\s*([^\s,]+)(?:\s+(?:AND|OR)\s+)?/i';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 151 - 159, The attribute-name regex in Item.php (the $pattern used before preg_match_all, and the parsing logic that fills $result['attributes'] in the same method) only uses (\w+) so multi-word names like "aspect ratio" are split; update the pattern to allow spaces and other common name chars (for example use a group like ([\p{L}\p{N}\s\-]+?) with the u flag) instead of (\w+), keep the value capture ([^\s,]+) as-is, and ensure you call strtolower(trim(...)) on the new capture ($match[1]) as before so multi-word names are normalized; leave remaining and preg_replace logic working with the new pattern.
🧹 Nitpick comments (1)
app/Models/Item.php (1)
176-177: Rename or re-documentincludeDeletedto match behavior.The implementation filters
items.deleted = $includeDeleted; it does not “include deleted” as described. This is easy to misuse via the new public API.Also applies to: 219-219, 274-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Item.php` around lines 176 - 177, The doc/param name $includeDeleted is misleading because the code filters items.deleted = $includeDeleted (it matches the deleted flag rather than “including” deleted items); rename the parameter to something like $deleted (or $matchDeleted) or update the PHPDoc to state “Whether to match items.deleted = true/false” and apply the same rename/documentation to every occurrence of $includeDeleted in the Item class (including the other two locations referenced), plus update all callers to pass the intended boolean meaning so the behavior and the public API are unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Item.php`:
- Around line 412-418: The sorting logic in the block using
getAttributeSortDefinitionId($sort) always orders by the text column
attribute_value, causing numeric/date attributes to sort lexicographically;
update the join/orderBy to choose the correct typed column based on the
attribute definition (use attribute_decimal for numeric, attribute_date for
date, fallback to attribute_value for text) before calling $builder->orderBy;
inspect the attribute definition via getAttributeSortDefinitionId($sort) or the
linked attribute metadata (same $sortAlias/{$sortAlias}_val context) and
substitute the column name used in orderBy accordingly (e.g.,
"{$sortAlias}_val.attribute_decimal" or "{$sortAlias}_val.attribute_date" when
appropriate).
- Around line 199-217: searchByAttributes() currently maps attribute names to
$definitionId via $definitionNameToId but never checks the provided
$definitionIds filter, so attribute keys outside the requested scope can match;
after resolving $definitionId in the foreach over $parsed['attributes'], add a
membership guard that skips this attribute unless $definitionIds is null/empty
or the resolved $definitionId is contained in $definitionIds (use strict
in_array or appropriate set lookup), ensuring only attributes within the
caller-provided $definitionIds are queried; update any unit tests covering
attribute-qualified searches to assert out-of-scope attributes are ignored.
---
Duplicate comments:
In `@app/Models/Item.php`:
- Around line 151-159: The attribute-name regex in Item.php (the $pattern used
before preg_match_all, and the parsing logic that fills $result['attributes'] in
the same method) only uses (\w+) so multi-word names like "aspect ratio" are
split; update the pattern to allow spaces and other common name chars (for
example use a group like ([\p{L}\p{N}\s\-]+?) with the u flag) instead of (\w+),
keep the value capture ([^\s,]+) as-is, and ensure you call
strtolower(trim(...)) on the new capture ($match[1]) as before so multi-word
names are normalized; leave remaining and preg_replace logic working with the
new pattern.
---
Nitpick comments:
In `@app/Models/Item.php`:
- Around line 176-177: The doc/param name $includeDeleted is misleading because
the code filters items.deleted = $includeDeleted (it matches the deleted flag
rather than “including” deleted items); rename the parameter to something like
$deleted (or $matchDeleted) or update the PHPDoc to state “Whether to match
items.deleted = true/false” and apply the same rename/documentation to every
occurrence of $includeDeleted in the Item class (including the other two
locations referenced), plus update all callers to pass the intended boolean
meaning so the behavior and the public API are unambiguous.
jekkos
pushed a commit
that referenced
this pull request
Mar 18, 2026
- Update regex pattern to support multi-word attribute names (e.g., 'aspect ratio') - Rename $includeDeleted to $matchDeleted for clarity (matches deleted flag value) - Add check to skip attributes not in caller-provided definitionIds filter - Use Unicode pattern modifier 'u' for international character support in attribute names This addresses CodeRabbit's feedback on PR #4442.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/Item.php`:
- Around line 416-425: The current attribute sorting always uses the text column
(`attribute_value`) which misorders numeric/date attributes; update the block
that handles attribute column sorting (the logic around
getAttributeSortDefinitionId and the joins using $sortAlias) to first fetch the
attribute definition/type for $sortDefinitionId (via the
Attribute/AttributeDefinition model or a small query against
attribute_definitions), map DECIMAL => "attribute_decimal", DATE =>
"attribute_date", and TEXT/DROPDOWN/CHECKBOX => "attribute_value", then change
the orderBy to use "{$sortAlias}_val.<chosen_column>" instead of always
"{$sortAlias}_val.attribute_value" so numbers and dates sort correctly while
preserving the existing join aliases and null-safe left joins.
objecttothis
previously approved these changes
Mar 19, 2026
objecttothis
left a comment
objecttothis
left a comment
Member
There was a problem hiding this comment.
These changes resolve my concerns. Probably see if anything coderabbitai says is worth changing, then merge.
jekkos
pushed a commit
that referenced
this pull request
Mar 19, 2026
- When sorting by attribute column, determine the attribute type - Use attribute_decimal for DECIMAL type - Use attribute_date for DATE type - Use attribute_value for TEXT/DROPDOWN/CHECKBOX types - This ensures numeric and date attributes sort correctly instead of lexicographically Fixes CodeRabbit feedback on PR #4442.
jekkos
force-pushed
the
fix/attribute-search-refactor
branch
from
e48767d to
a17e49b
Compare
jekkos
pushed a commit
that referenced
this pull request
Mar 19, 2026
- Update regex pattern to support multi-word attribute names (e.g., 'aspect ratio') - Rename $includeDeleted to $matchDeleted for clarity (matches deleted flag value) - Add check to skip attributes not in caller-provided definitionIds filter - Use Unicode pattern modifier 'u' for international character support in attribute names This addresses CodeRabbit's feedback on PR #4442.
jekkos
pushed a commit
that referenced
this pull request
Mar 19, 2026
- When sorting by attribute column, determine the attribute type - Use attribute_decimal for DECIMAL type - Use attribute_date for DATE type - Use attribute_value for TEXT/DROPDOWN/CHECKBOX types - This ensures numeric and date attributes sort correctly instead of lexicographically Fixes CodeRabbit feedback on PR #4442.
jekkos
pushed a commit
that referenced
this pull request
Mar 19, 2026
The following files had changes mixed in from PRs #4442 and others that do not belong in this Travis CI to GitHub Actions migration PR: - app/Controllers/Attributes.php - app/Controllers/Expenses.php - app/Controllers/Items.php - app/Controllers/Receivings.php - app/Helpers/tabular_helper.php - app/Language/en-GB/Attributes.php - app/Language/en/Attributes.php - app/Models/Attribute.php - app/Models/Item.php - app/Views/expenses/form.php - app/Views/receivings/form.php These have been reverted to match the master branch state.
| $sortColumn = "{$sortAlias}_val.attribute_value"; // default to text | ||
|
|
||
| if (isset($definitionInfo[$sortDefinitionId])) { | ||
| $defType = is_array($definitionInfo[$sortDefinitionId]) ? ($definitionInfo[$sortDefinitionId]['type'] ?? TEXT) : TEXT; |
Member
There was a problem hiding this comment.
I think the fallback prevents a problem here, but it appears this doesn't account for DROPDOWN attribute types, which are also attribute_value.
Member
Author
Performance Analysis: Pre-query + WHERE IN vs GROUP_CONCAT + HAVINGGiven the performance regression reported in #4519, I did a formal analysis comparing the two search approaches to validate this PR's architecture. The Two ApproachesWhy This PR is FasterCommon case (simple text search with attributes enabled): Master execution path:
Total work: O(items x avg_attributes) - materialize, group, concat, then filter This PR execution path:
Total work: O(matching_items) - filter early, then minimal display work Estimated speedup: 3-10x on datasets with 10K+ items, scaling with dataset size. Worst case (multi-attribute AND search: "color:blue AND size:large AND brand:nike"): Master: Same single query with compound HAVING conditions. Must still materialize ALL items with ALL attributes, then filter. AND logic on concatenated strings is fragile (pattern matching on aggregated text). This PR: 3 subqueries + 1 main = 4 total queries. Each subquery is indexed and returns a small set of item_ids. "N+1" Concern AnalysisThis is not classic N+1. N is the number of attribute predicates (typically 2-5), NOT the number of items (10K+). Even 10 subqueries = 10 round-trips = ~50ms, versus 2-10 seconds for the GROUP_CONCAT approach on large datasets. The query count is bounded and small. GROUP_CONCAT HAVING Has Unfixable Correctness BugsBeyond performance, the current master approach has correctness issues that cannot be fixed without this architectural change:
Optimization Opportunities (Non-Blocking)Two improvements that would make this PR even faster:
VerdictThis PR is provably faster for all practical OSPOS workloads and fixes correctness issues that are architecturally unfixable in the GROUP_CONCAT approach. The rebased branch also includes the |
jekkos
pushed a commit
that referenced
this pull request
May 16, 2026
- Update regex pattern to support multi-word attribute names (e.g., 'aspect ratio') - Rename $includeDeleted to $matchDeleted for clarity (matches deleted flag value) - Add check to skip attributes not in caller-provided definitionIds filter - Use Unicode pattern modifier 'u' for international character support in attribute names This addresses CodeRabbit's feedback on PR #4442.
jekkos
pushed a commit
that referenced
this pull request
May 16, 2026
- When sorting by attribute column, determine the attribute type - Use attribute_decimal for DECIMAL type - Use attribute_date for DATE type - Use attribute_value for TEXT/DROPDOWN/CHECKBOX types - This ensures numeric and date attributes sort correctly instead of lexicographically Fixes CodeRabbit feedback on PR #4442.
jekkos
force-pushed
the
fix/attribute-search-refactor
branch
from
ad65127 to
e3c2175
Compare
2 tasks
added 7 commits
June 8, 2026 20:03
- Add new search_by_attributes() method that returns matching item_ids - Refactor search() method to use subquery approach instead of HAVING LIKE - Fix pagination count issue (#2819) - get_found_rows() now returns correct count - Enable combined search (attributes OR regular fields) - GROUP_CONCAT still used for display but removed from search/count logic - Fixes #2407 - multi-attribute search now works correctly Related: #2819, #2407, #2722, #2919
Phase 2 implementation: - Add SHOW_IN_SEARCH constant (value 8) to Attribute model - Update Items controller to include searchable attributes when search_custom filter is enabled - Add language strings for the new visibility flag - Update Attributes controller to include new flag in form This allows attributes to be searchable even if they are not displayed in the items table, addressing issue #2919. Fixes #2919
… by attribute columns Phase 3 - Multi-attribute Search: - Add parse_attribute_search() method to parse search syntax like 'color: blue size: large' or 'color:blue AND size:large' - Update search_by_attributes() to support AND/OR logic for multiple attribute queries - Add search_by_attribute_value() private method for single value search Phase 4 - Sort by Attribute Columns: - Add get_attribute_sort_definition_id() method to detect attribute column sorting - Update Item::search() to join attribute tables when sorting by attribute columns - Update tabular_helper to make attribute columns sortable - Add sanitizeSortColumnAttribute() method in Items controller to validate attribute definition IDs as sort columns Fixes #2722 - sorting by attribute columns now works
- Add Item::ALLOWED_SORT_COLUMNS constant for allowed sort columns - Use constant in sanitizeSortColumnAttribute() instead of inline array - Enables reuse across the codebase for sort column validation
…ns helper - Add item_sort_columns() helper function in tabular_helper.php - Helper returns all sortable columns including dynamic attribute IDs - Remove duplicate sanitizeSortColumnAttribute method from Items controller - Remove unused ALLOWED_SORT_COLUMNS constant from Item model - Reuses existing sanitizeSortColumn method from Secure_Controller
- Fix PHPDoc comment format in item_sort_columns() helper - Remove duplicate blank line after properties in Item model - Remove unused variable assignment in Items controller
- Rename methods to camelCase (PSR-12): parseAttributeSearch, searchByAttributes, searchByAttributeValue, getAttributeSortDefinitionId - Rename variables to camelCase (PSR-12) - Add DATE attribute search support in searchByAttributeValue - Fix get_definitions_by_flags to request typed payload (second param true) - Handle both array and string return shapes for definition info - Fix term-only search to merge with attribute matches instead of replacing - Use strict emptiness checks (=== '' instead of empty()) - Sanitize definition_ids before SQL interpolation (array_map intval) - Include DATE in attribute search conditions
added 4 commits
June 8, 2026 20:03
- Update regex pattern to support multi-word attribute names (e.g., 'aspect ratio') - Rename $includeDeleted to $matchDeleted for clarity (matches deleted flag value) - Add check to skip attributes not in caller-provided definitionIds filter - Use Unicode pattern modifier 'u' for international character support in attribute names This addresses CodeRabbit's feedback on PR #4442.
- When sorting by attribute column, determine the attribute type - Use attribute_decimal for DECIMAL type - Use attribute_date for DATE type - Use attribute_value for TEXT/DROPDOWN/CHECKBOX types - This ensures numeric and date attributes sort correctly instead of lexicographically Fixes CodeRabbit feedback on PR #4442.
…tColumn - sanitizeSortColumn expects nested array format [['col' => 'label'], ...] - Reuse item_headers() as base and add attribute columns - This maintains DRY principle by not redefining columns twice
- Add 'show_in_search' and 'show_in_search_visibility' to all language files - Translated: de-DE, es-ES, fr, it - Placeholder English for remaining languages (awaiting translation) This ensures the SHOW_IN_SEARCH attribute flag works in all locales.
jekkos
force-pushed
the
fix/attribute-search-refactor
branch
from
e3c2175 to
303be38
Compare
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
2 participants
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 by CodeRabbit
New Features
Improvements
Localization