Comment 20 for bug 1152863

With an extra release cycle to look at this again, I would like to re-raise my previous concerns about the approach taken here. Specifically, instead of using regular expressions to mangle a mechanically constructed query string at the surface (it's a rule: users will always find a way to confuse complex REs), leverage existing functionality meant for exactly this purpose. (See comment 14 above.)

Aside from my bias toward that particular implementation path, I'm very worried about the use of translation strings here. Their use here is akin to embedding html elements in translatable strings. You're mixing the concerns and purposes of code and translation markers in ways that are well outside the existing use pattern in Evergreen (and, in my understanding and opinion, against recommended i18n design patterns in general), and my fear is that someone down the road will accidentally break this feature because they didn't realize that those translation markers were special in some way, and that breakage will persist for quite a while. This is likely because working on the OPAC should not mean thinking about search syntax and parsing; you think about that when you're working on the search backend and query parser.

If, instead, the search code was intentionally taught to know about and deal with alternate boolean operator spellings, those working on the search code would assist in its maintenance, and those working on the OPAC would not have to avoid breaking the functionality.

In other words, to be blunt for the purpose of clarity, the brittleness of the design and the layer at which it is implemented mean that this will not likely be well-maintained in the future by those that work on the search /or/ user-interface portions of Evergreen.

Even if nothing is done to address the design and implementation -- I'm not the only committer, so another may pick this up to merge -- I would appreciate some discussion of these concerns. If anyone believes I am going overboard and seeing issues where there are none -- I'm only human -- I would be happy to hear why. I'm more than willing to be convinced that the approach here is the best we can or should do, but I currently feel pretty strongly that we can do better. I also believe it would not take any more code change (252 line inserted or deleted by the signed-off branch) to implement, and perhaps less.

Thanks, and please chalk up any negative tone to the medium. I'm only posting to discuss the code, not to attack the implementors.