-
Notifications
You must be signed in to change notification settings - Fork 913
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
pkp/pkp-lib#10139 fix Author search pulls in keywords too #4380
base: stable-3_3_0
Are you sure you want to change the base?
Conversation
@jonasraoni, could you review this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hafsa-Naeem I've left two comments, but as I'll replace this code with #4224, I think you just need to update the $sqlWhere .= ' AND (o.type & ?) != 0';
part.
if ($i > 0) $sqlWhere .= ' AND o0.object_id = o'.$i.'.object_id AND o0.pos+'.$i.' = o'.$i.'.pos'; | ||
|
||
$sqlWhere .= $i > 0 ? ' AND ' : ''; | ||
$sqlWhere .= 'k'.$i.'.keyword_text LIKE ?'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might change a bit the behavior (e.g. _
, a wildcard, would be searched with an exact match previously), but I didn't inspect further, in general it should be ok...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hafsa-Naeem The problem should be fixed after the latest update, but please, check why the tests are failing (perhaps it's missing a rebase?!).
Just one thing... Alec asked to avoid touching sensible parts of the code on the stable
branches, to avoid breaking the toy (our tests are not covering everything), unless it's something really important. So, can you drop the not needed updates? I guess only the line 50 is needed (I know it's painful 😤).
Then, you can create the PRs for the other applications (if the problem exists on them) and port to stable-3_4_0
and main
. (or if you prefer, I can just cherry-pick the commits).
fix for #10139