Re: [Gallery-core] AND search finished, request for review

I did a cursory review of the CL and had some comments (not covering
stuff that Alan already covered-- I think):

* The point of having versioned interfaces is that you can add new
functionality while maintaining backwards compatibility.  If you're
adding a new method to SearchUtilities, this means that all modules who
use it are dependent on the latest search module (else PHP will choke on
the missing method when you call it in GalleryCommentSearch).  So the
way to do this would be to create GallerySearchInterface_1_1.class and
have modules switch to supporting that interface.  Older versions of the
search module won't call these modules because they'll no longer get
served up from the factory.  Newer versions of the search module *will*
call them and can do the and search directly.  If you want a module that
supports both G2.1 and G2.2 you can implement both interfaces.  This
gets rid of  SearchUtilities::doAndSearch and gives you broad compatibility.

* CoreSearchTest::testSearchAllSomeFieldsAnd (and others) shouldn't be
gated on a boolean value; it should find a way to test the code (moot
after you satisfy the above point, but still a good philosophy).

* Nice tests for malicious search queries!

* The last assertion in TestSmartyModifierHighlightWithXmlTags should be
broken up into several smaller ones that verify each individual feature
that you're checking for here. It looks like you've got a lot of
unnecessary extra overhead in there; but if you break it out into
smaller tests it should be obvious if there is extra value in having 8
needles.  This should make it clearer that your change doesn't reopen
bug 1422405 [1], which it looks like it doesn't.

* modules/search/HighlightResults.js can be deleted.  Mark it as R in
the MANIFEST file.


rest looks pretty good to me (modulo Alan's comments).
-Bharat

[1]:
http://sourceforge.net/tracker/index.php?func=detail&aid=1422405&group_id=7130&atid=107130

Alan Harder wrote:

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]