Skip to content

feat(search): include Template Variable values in manager search#16902

Open
Ibochkarev wants to merge 2 commits intomodxcms:3.xfrom
Ibochkarev:feat/16703-tv-values-in-manager-search
Open

feat(search): include Template Variable values in manager search#16902
Ibochkarev wants to merge 2 commits intomodxcms:3.xfrom
Ibochkarev:feat/16703-tv-values-in-manager-search

Conversation

@Ibochkarev
Copy link
Collaborator

What does it do?

  • Adds search in Template Variable (TV) resource values (modTemplateVarResource.value) when "Search in content" is enabled in manager search.
  • Refactors resource search into smaller methods: getResourceContextKeys, buildResourceSearchCriteria, getResourceIdsMatchingTvValues, applyResourceSearchSortBy, formatResourceSearchResult.
  • Applies relevance ordering: exact pagetitle match → pagetitle starts with query → other fields (longtitle, alias, description, introtext) → content → TV value match → newest first (createdon DESC).
  • Limits TV-matched resource IDs to getMaxResults() * 2 and deduplicates to avoid oversized IN clauses.

Why is it needed?

Manager quick search did not look inside TV values, so resources that matched only by custom TV fields were not found. This change includes TV values in the search scope and orders results by relevance.

How to test

  1. Create a resource and set a Template Variable to a distinct value (e.g. "my-unique-tv-value").
  2. In the manager, use the global search and query "my-unique-tv-value".
  3. With "Search in content" enabled, the resource should appear in results.
  4. Verify ordering: search for a term that matches both a resource pagetitle and a TV on another resource; the one with pagetitle match should list first.

Related issue(s)/PR(s)

Resolves #16703

- Add search in modTemplateVarResource.value when quick_search_in_content is enabled
- Refactor searchResources into smaller methods: getResourceContextKeys,
  buildResourceSearchCriteria, getResourceIdsMatchingTvValues,
  applyResourceSearchSortBy, formatResourceSearchResult
- Apply relevance ordering: exact pagetitle, pagetitle prefix, other fields,
  content, TV match, then createdon DESC
- Limit TV-matched resource IDs to getMaxResults()*2 and deduplicate

Resolves modxcms#16703
@Ibochkarev Ibochkarev changed the title feat(Search): include Template Variable values in manager search feat(search): include Template Variable values in manager search Feb 25, 2026
@Ibochkarev Ibochkarev marked this pull request as ready for review February 25, 2026 16:53
@biz87
Copy link

biz87 commented Feb 25, 2026

Code Review

Summary

Adds TV value search to manager quick search, refactors searchResources() into 5 focused methods, and implements relevance-based sorting (exact pagetitle > starts with > other fields > content > TV match > newest). Single file, +121/-33.

Issues

Search.php:131$this->query is used in LIKE without escaping wildcard characters.
If a user searches for a string containing % or _, they are interpreted as LIKE wildcards. This is not SQL injection (values are quoted by xPDO), but it affects search correctness. The same issue exists in the original code, but since the TV query is new, it's worth fixing at least there:

$escaped = addcslashes($this->query, '%_');
$c->where(['value:LIKE' => '%' . $escaped . '%']);

Non-blocking — this is a pre-existing issue.

Search.php:185-196 — Sort expressions duplicate field logic from buildResourceSearchCriteria().
If a new search field is added later, it must be updated in both places. Not a bug, but a maintenance concern. A comment linking these two locations would help.

Suggestions

  • Search.php:132 — The limit getMaxResults() * 2 for TV query is a magic number. A single resource can have many TVs, so for sites with heavy TV usage this may miss results. Consider GROUP BY contentid in the query to limit by unique resource IDs instead.

  • Search.php:179 — Parameter $c has PHPDoc type but no native type hint, unlike the other new methods.

Assessment

Clean refactoring — the monolithic method is split into logical parts, each with PHPDoc and return types. TV search uses a separate query with deduplication and limit to prevent oversized IN clauses. Relevance sorting is a solid UX improvement. All queries use xPDO criteria API, ::class constants, and getIterator. PHPStan clean.

Verdict

Approve — issues are non-blocking, PR is ready to merge.

@Ibochkarev Ibochkarev added the feature Request about implementing a brand new function or possibility. label Feb 26, 2026
@halftrainedharry
Copy link
Contributor

  • when "Search in content" is enabled in manager search

It's a bit counterintuitive that the quick_search_in_content setting is now also used to search in TVs.

  • exact pagetitle match → pagetitle starts with query

So if the pagetitle contains the search term but not at the beginning, then this resource is not at the top of the result-list. Why?

  • Limits TV-matched resource IDs to getMaxResults() * 2

For what reason is a multiplying factor of 2 used here?


  • This PR doesn't handle the default values of TVs. If the search term is the default value of a TV, the resources that use that default value are not in the result.

  • In my opinion, it would be cleaner and faster to use a LEFT JOIN for the TV table, than to run 2 queries (the first just to get some resource-IDs of matching TV values).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Request about implementing a brand new function or possibility.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include the values of TVs when using manager's search functionality

3 participants