search

jquery / jquery-wp-content

WordPress themes and plugins for the jQuery sites
GNU General Public License v2.0
253 stars 169 forks source link

All: Add Relevanssi plugin, remove broken search algorithm #408

Closed mgol closed 6 years ago

mgol commented 6 years ago

A rebased #361

Krinkle commented 6 years ago

@mgol Seems fine from a quick scan and from WordPress plugin/mu perspective. As for its actual PHP code, I haven't done a complete security/integrity analysis. Neither can I say for certain that it "just work" after merely loading the files.

I'd normally expect to first install the plugin without enabling, then have the updater create the tables and create the initial index, and then enable the plugin and its ability to keep the index up-to-date.

But, the code does seem to indicate that it has the capability to do it all on-the-fly. Assuming that's all good, my only concern would be how well it is able to do that in a way that won't upset the web server during the initial requests when it is still busy rebuilding. E.g. does it use a lock to make sure only one request performs the lazy install and the rest no-ops meanwhile? That I'm not sure of.

Anyhow, lacking a better way to test it, I suppose let's just try it?

By the way, how does deployment work for jquery-wp-content? I'd hope that commits here immediately update all staging sites, but not production. And that production update either depends on a tag in this repo, or a tag in all content repos?

mgol commented 6 years ago

By the way, how does deployment work for jquery-wp-content? I'd hope that commits here immediately update all staging sites, but not production. And that production update either depends on a tag in this repo, or a tag in all content repos?

This is how it works for the API pages at least but I've never made sure the same applies to jquery-wp-content.

dmethvin commented 6 years ago

I did a quick look-through, mainly to ensure there were no SQL injection issues. As long as relevanssi_variables are okay there shouldn't be a problem. I think we should just go for it.

PaulAnnekov commented 6 years ago

3 years... I thought it won't be ever merged.

mgol commented 6 years ago

@PaulAnnekov Sorry for the trouble. Our content team is seriously understaffed and the few people that maintain it are not always available.

BTW, do you perhaps know what might be causing pagination issues after landing this PR? See this comment for more details.