Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix search form ending in dash error #2727

Open
wants to merge 1 commit into
base: 4
Choose a base branch
from
Open

Conversation

3Dgoo
Copy link
Contributor

@3Dgoo 3Dgoo commented Mar 27, 2022

On the search form if a user's search ends in a dash it will cause a server error.

This change fixes this problem by trimming dashes and spaces from the end of the search term.

On the search form if a user's search ends in a dash it will cause a server error.

This change fixes this problem by trimming dashes and spaces from the end of the search term.
Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To trim whitespaces is probably legitimate here, but I don't think we should be trimming potentially meaningful characters. It seems the problem here is that the hyphen in the keyword isn't escaped properly for the db query where it uses a minus to create a not predicate. So I'd prefer a fix taking that into account.

@muskie9
Copy link
Contributor

muskie9 commented Mar 27, 2022

I've been looking at another similar scenario where FullTextSearch (site search) against something like ABC-123, where ABC-123 is in the content zone it yields no results. This is due to the not predicate from what I've seen thus far.

I'm wondering if making the modifiers more flexible as right now I believe it simply looks for + or - and assumes you know what those do within the keyword string and how it affects the query. As a developer I didn't know that was the case so I'd guess CMS and end users would have no idea.

@3Dgoo
Copy link
Contributor Author

3Dgoo commented Mar 28, 2022

The code that follows the initial keyword fetch shows that the code uses the - to create a "not" search condition:

        $andProcessor = function ($matches) {
            return ' +' . $matches[2] . ' +' . $matches[4] . ' ';
        };
        $notProcessor = function ($matches) {
            return ' -' . $matches[3];
        };
        $keywords = preg_replace_callback('/()("[^()"]+")( and )("[^"()]+")()/i', $andProcessor, $keywords);
        $keywords = preg_replace_callback('/(^| )([^() ]+)( and )([^ ()]+)( |$)/i', $andProcessor, $keywords);
        $keywords = preg_replace_callback('/(^| )(not )("[^"()]+")/i', $notProcessor, $keywords);
        $keywords = preg_replace_callback('/(^| )(not )([^() ]+)( |$)/i', $notProcessor, $keywords);
        $keywords = $this->addStarsToKeywords($keywords);

I think escaping the - would break that code.

On the other hand, stripping any dashes on the end of the whole search string shouldn't break anything.

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.

3 participants