Skip to content

Conversation

@d0ubIeU
Copy link
Contributor

@d0ubIeU d0ubIeU commented Dec 13, 2025

remove old and unused part.
breadcrumbs for Category now in show.twig (Commit 409086a)

Summary by CodeRabbit

  • Refactor
    • Removed breadcrumb navigation display from the search interface completely
    • Changed search form submission method from POST to GET request, enabling search parameters to be passed via URL query string
    • Updated search term parameter retrieval to source from URL query string instead of POST body while maintaining input sanitization and length validation

✏️ Tip: You can customize this high-level summary in your review settings.

Unifying the search on form method="get" to make search results accessible for bookmarks and sharing via link.
fix that results available on normal and advanced search
remove old unused breadcrumb part
@coderabbitai
Copy link

coderabbitai bot commented Dec 13, 2025

Walkthrough

The search functionality is modified to use HTTP GET requests instead of POST. The search form method is updated, and search.php is changed to read the search term from the query string rather than the request body. The breadcrumb component is removed from the template.

Changes

Cohort / File(s) Summary
Search form HTTP method update
phpmyfaq/assets/templates/default/index.twig
Search form method changed from POST to GET; breadcrumb block removed entirely
Search parameter source update
phpmyfaq/search.php
Search term input source changed from POST body (request->request) to query string (request->query); sanitization and truncation logic preserved

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify that changing from POST to GET search does not introduce unintended side effects (e.g., query string length considerations, caching behavior)
  • Confirm parameter sanitization remains intact after the source switch
  • Ensure breadcrumb removal is intentional and doesn't break dependent functionality

Possibly related PRs

  • PR #3806: Implements the same HTTP method conversion for the search form from POST to GET with corresponding parameter source updates in search.php

Suggested reviewers

  • thorsten

Poem

🐰 A form that hops from POST to GET,
Through query strings the searches set,
Breadcrumbs cleared from the template trail,
Simpler searches shall prevail!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title states it removes an old, unused breadcrumb part, but the changeset also includes a significant form method change from POST to GET in the search form, which is not mentioned in the title. Update the title to reflect both major changes, such as 'fix: remove breadcrumb and change search form to GET method' or focus only on breadcrumbs if the form change is unintended.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 409086a and 3b4f7e6.

📒 Files selected for processing (2)
  • phpmyfaq/assets/templates/default/index.twig (1 hunks)
  • phpmyfaq/search.php (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php

📄 CodeRabbit inference engine (AGENTS.md)

Use PSR-12 coding standards for PHP code

Files:

  • phpmyfaq/search.php
**/*.{php,ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{php,ts,tsx,js,jsx}: Use semicolons at the end of each statement
Use single quotes for strings
Follow best practices for localization, such as using placeholders for dynamic content and avoiding hard-coded strings

Files:

  • phpmyfaq/search.php
🧬 Code graph analysis (1)
phpmyfaq/search.php (1)
phpmyfaq/src/phpMyFAQ/Filter.php (2)
  • Filter (29-329)
  • filterVar (69-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: phpMyFAQ 8.4 Test on ubuntu-latest
  • GitHub Check: phpMyFAQ 8.3 Test on ubuntu-latest
  • GitHub Check: phpMyFAQ 8.5 Test on ubuntu-latest
  • GitHub Check: phpMyFAQ 8.6 Test on ubuntu-latest
🔇 Additional comments (2)
phpmyfaq/assets/templates/default/index.twig (1)

108-113: GET search form looks correct; verify nothing still expects POST.

Switching to method="get" matches the server-side change and makes search URLs shareable. Please just sanity-check any JS (autocomplete) and backend routing that may have assumed POST submissions.

phpmyfaq/search.php (1)

59-65: Normalize search term to empty string when null to prevent unexpected behavior.

Request::query->get('search') may return null when the parameter is missing. If Filter::filterVar() propagates this null value, it could cause issues: (a) Strings::substr() may not handle null gracefully, and (b) null !== '' evaluates to true, causing unexpected fulltext search logic. Use null coalescing to ensure the variable is always a string:

-$inputSearchTerm = Filter::filterVar($request->query->get('search'), FILTER_SANITIZE_SPECIAL_CHARS);
+$inputSearchTerm = (string) (Filter::filterVar($request->query->get('search'), FILTER_SANITIZE_SPECIAL_CHARS) ?? '');

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thorsten
Copy link
Owner

@d0ubIeU thanks but already done

@thorsten thorsten closed this Dec 13, 2025
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.

2 participants