This comment probably doesn't bestow any great enlightenment on any reasonable reader.

$tagCache is never defined as a property, I think? Prefer to define properties even if your access patterns don't technically require that they be defined.

Actually, since it isn't defined as a static property, I don't think this works at all? Does self::$tagCache[$x] = $y on an un-initialized property create it as a static property?

On my system (PHP 7.1), it looks like it doesn't: I get a fatal error instead:

PHP Fatal error: Uncaught Error: Access to undeclared static property: C::$value in /Users/epriestley/test.php:7

Does this need to be cached? We normally (always?) hit this cache only once per page, right? So just running the query without caching would have the same performance?

If this must use a cache, it should use PhabricatorCaches::getRequestCache(), not static. In the future, we may run multiple different requests (for performance) or multiple different Phabricator instances (for reducing memory footprints) in the same process. In this case, the scope of a static variable might span different viewers or instances. We don't do this today, but I want to leave room to do it in the future if it makes sense to pursue it.

So the scope of static means that it's only safe to use to store data which does not depend on the viewer, instance, or any configuration. If data does depend on the viewer/instance/config, use PhabricatorCaches::getRequestCache() instead, which works like static but is guaranteed to persist for only the scope of a logical request (e.g., a single web request, or a single task execution in the daemons).

Oh, this is happening on the indexing pathway. The desire for a cache makes sense, then.

I think this is a change of semantics, causing a search for "apple" to find objects tagged with "apple". I don't want this change in the upstream: I think it's confusing on its own, exceptionally confusing when projects are renamed (renaming projects does not rebuild the index, making it stale), and is also a meaningful semantic change between the MySQL and ElasticSearch engines. As long as ElasticSearch is in the upstream, I'd like the different engines to work the same way as much as possible.

But, particularly, I think this leads to cases where it isn't obvious why results are present in the result set (e.g., the document is tagged with a project which previously had one of the query terms in one of its slugs).

I'm okay with an upstream change to make searching for #asdf mean "search for documents tagged asdf", not "search for documents containing text #asdf" (see T7860) but I think making a search for "differential" imply that the result set should include tagged documents isn't desirable, especially when the index isn't rename-resistant.

I'd even be potentially OK with a change like: do a second search for all the terms the user submitted to see if they could match projects, then provide the user some hint text about using "#" to search for projects, e.g. "Searched for the text 'differential'. To search for documents tagged with Differential, search for 'Differential' instead.", similar to the Google spelling corrector hint.

This also seems like a semantic change, making fields like "Authors" and "Owners" optional instead of required? Maybe I'm misunderstanding how SHOULD and MUST work. If I query for "authored by X", I think the results should only include results which are authored by X.

Again, I'd be fine with a change like a hint that says "nothing matched, try a more-general query, perhaps by removing the authors constraint, click here to query without 'authors: x'" if that's really what's going on here, but I'd like to see some evidence that users are really having trouble with this first.

(But I might just be totally misunderstanding what's going on here.)

Okay, from below, I think I have no clue what's happening here. Are you just trying to boost results with matches in other fields? Since none of the other fields have text in them (I think?) this seems non-useful? It just makes "phid" hit every document, "user" hit every document with an author, etc?

Or maybe I'm just wrong about that too.

Note: in the wmf fork, the version was configurable. I had assumed that was already upstreamed but apparently not.

Since D17384 reworks the config for this and other values, I've hard-coded it here, just to get this change merged cleanly and separately.

This method essentially just looks up the slugs for projects by phid so that we index searchable text rather than only the relationship phid references.

Now it occurs to me that I should document this in the source ;)

Sorry that was my mistake when splitting out the patches, it's supposed to be static class property.

Does this need to be cached? We normally (always?) hit this cache only once per page, right? So just running the query without caching would have the same performance?

It's called once per document when indexing.

If this must use a cache, it should use PhabricatorCaches::getRequestCache(), not static. In the future, we may run multiple different requests (for performance) or multiple different Phabricator instances (for reducing memory footprints) in the same process. In this case, the scope of a static variable might span different viewers or instances. We don't do this today, but I want to leave room to do it in the future if it makes sense to pursue it.

ok, I'll switch to using getRequestCache...

Okay, from below, I think I have no clue what's happening here. Are you just trying to boost results with matches in other fields? Since none of the other fields have text in them (I think?) this seems non-useful? It just makes "phid" hit every document, "user" hit every document with an author, etc?

That wasn't my intent, however, after testing, it appears that you are right about this.

I think I'm going to pull out some of these changes into a subclass for wikimedia-specific experiments and I'll update this revision to be mostly similar to the upstream version, just with updated es5 syntax.