Fix #2349 better replace for or and and keywords

[BettyIsBoop]

Resolves #2349

[lairdshaw]

This fix fails in three scenarios:

1. One or more spaces precede the opening "and" or "or" (it fails to remove the "and" or "or").
2. The string starts with more than one "and" or "or" (it only removes the first).
3. The string consists in a single "and" or "or" (it fails to remove that "and" or "or").

The solution provided in this comment works in all of those scenarios [edit: for the first, only when the $keywords argument is replaced by trim($keywords)], and should, I think, be preferred:
#2349 (comment)

[RevertIT]

it should use trim_blank_chrs() to match all varieties of blank characters. eg: "invisible blank name" characters.

[BettyIsBoop]

One or more spaces precede the opening "and" or "or" (it fails to remove the "and" or "or").

Same with current one

The string starts with more than one "and" or "or" (it only removes the first).

Same with current one

The string consists in a single "and" or "or" (it fails to remove that "and" or "or").

OK

Personally i use on my forum because current solution broke more times. I check with regexp.

Simple issue open on 2016 … still exist 7 years after, Let Me Laugh …

[BettyIsBoop]

This fix fails in three scenarios:

1. One or more spaces precede the opening "and" or "or" (it fails to remove the "and" or "or").

Like before : trim remove space

2. The string starts with more than one "and" or "or" (it only removes the first).

Like current

3. The string consists in a single "and" or "or" (it fails to remove that "and" or "or").

Fixed

The solution provided in this comment works in all of those scenarios [edit: for the first, only when the $keywords argument is replaced by trim($keywords)], and should, I think, be preferred: #2349 (comment)

Done

[lairdshaw]

This fix fails in three scenarios:

1. One or more spaces precede the opening "and" or "or" (it fails to remove the "and" or "or").

Like before : trim remove space

You're right - I'd missed that. Arguably, as @RevertIT suggests, that trim() should be converted to trim_blank_chrs(), but that's probably not necessary for this PR.

The solution provided in this comment works in all of those scenarios [edit: for the first, only when the $keywords argument is replaced by trim($keywords)], and should, I think, be preferred: #2349 (comment)

Done

Untested but LGTM.

[Sama34]

This seems to work when searching for "andy", it finds the following thread titles :

Tempore technologia paternoster tandy
Andyravida technologia tristique urna nullandy
Computandytor quis viverra ipsum venenatis

When searching for "ORAS", it finds the following thread titles :

ORAS, Pokemon nunc risus aliquam risus
Eget continentes pellentesque cinque ORAS
Andyravida technoloRASgia tristique urna nullandy

Before the patch the same search resulted in the "minimum length" error.

We appreciate your contribution !

[RevertIT]

@Sama34 Try searching with quotes ' " and special characters to see if it will break syntax, can't really test it myself nowadays

[Shnoulle]

You're right - I'd missed that. Arguably, as @RevertIT suggests, that trim() should be converted to trim_blank_chrs(), but that's probably not necessary for this PR.

It's for user entered data when search, a simple trim for spaced added by copy/paste : i really don't think need more.

[RevertIT]

It's for user entered data when search, a simple trim for spaced added by copy/paste : i really don't think need more.

Well, it's actually needed, trim doesn't trim invisible characters as shown below:
https://3v4l.org/ObKdY#v8.2.6

Output:

Trim blank character
string(19) "   f   "


Trim blank character via chrs function 
string(1) "f"


Trim normal space 
string(1) "f"

[Shnoulle]

It's for user entered data when search, a simple trim for spaced added by copy/paste : i really don't think need more.

Well, it's actually needed, trim doesn't trim invisible characters as shown below: https://3v4l.org/ObKdY#v8.2.6

1. It's another issue
2. You think mybb user use u+XXXX when do a search ? And if by mistake : they enter an invisible character : they don't find what the want … ,not a real issue.

This fix one issue, and not another one.

[Sama34]

@Sama34 Try searching with quotes ' " and special characters to see if it will break syntax, can't really test it myself nowadays

Searching for the following yields the same (positive) results as my previous post :

"andy"
"oras"

Searching for the following yields a Sorry, but no results were returned ... error result:

'andy"
'oras"
'andy'
'oras'
"andy'
"oras'
"&andy%"
"&oras%"
/oras

I stopped testing after all these terms. My environment is:

MyBB 1.8.33
PHP 8.2.6
SQL MySQL (PDO) 8.0.33-0ubuntu0.22.04.2

[BettyIsBoop]

@Sama34 Try searching with quotes ' " and special characters to see if it will break syntax, can't really test it myself nowadays

Searching for the following yields the same (positive) results as my previous post :

I stopped tested after all these terms. I'm testing on: MyBB 1.8.33 PHP 8.2.6 SQL MySQL (PDO) 8.0.33-0ubuntu0.22.04.2

I really think it's totally unrelated to this issue …

Did you compare with current ? Dis you compare with "toty" or 'toty" etc ?

[RevertIT]

You think mybb user use u+XXXX when do a search ? And if by mistake : they enter an invisible character : they don't find what the want … ,not a real issue.

Doesn't change the fact it's better to implement now than forget down the road if the native function exits.

[lairdshaw]

Doesn't change the fact it's better to implement now than forget down the road if the native function exits.

Yeah, given that it's got a benefit (handling more cases of whitespace), the question seems to be "Is there any reason not to?" The only potentially legit one I can think of is "It's less performant", but given that this isn't performance-critical code, and the performance impact is anyway very small, that doesn't seem to be an actually legit reason. So, I'm in favour of the change, but also fine with it being achieved via a separate issue+PR. My 2c.

[Eldenroot]

You think mybb user use u+XXXX when do a search ? And if by mistake : they enter an invisible character : they don't find what the want … ,not a real issue.

Doesn't change the fact it's better to implement now than forget down the road if the native function exits.

I agree. @BettyIsBoop please could you edit your PR to cover all? Thx!

I hope this will be merged into 1.8.35 :)

[dvz]

Can none of the listed entities be part of a valid search phrase (perhaps in other languages/scripts)?

If there are no known issues, we can keep using the inbuilt functions - if there's a problem, these can be replaced across the application later.

[BettyIsBoop]

You think mybb user use u+XXXX when do a search ? And if by mistake : they enter an invisible character : they don't find what the want … ,not a real issue.

Doesn't change the fact it's better to implement now than forget down the road if the native function exits.

I agree. @BettyIsBoop please could you edit your PR to cover all? Thx!

I hope this will be merged into 1.8.35 :)

Done