Unify cache clearing logic / clear Views cache after adding joins.

TL/DR: this should be useful for at least people wanting to work with Views and adding joins, to not get confused.

Original issue:

After adding a join to a Data table, it does not show up as a relationship in the table view. (This combined with some other real/percieved bugs in the views related code, made for an interesting day chasing wild geese.)

So I wanted to fix the views-data cache being cleared whem adding a join.

But it was hard to decide what to fix where exactly, because the cache clearing code isn't very consistent.

This evolved into:

Just go through the whole (DataTable class + UI form submit) code and implement cache clearing that is more consistent than now. This unfortunately does mean that the patch is largish and unstructured.

The end result:

• Cache clearing (that is: calling DataTable::clearCache()) is mostly done from methods inside DataTable; this is preferred over first calling DataTable methods and clearing caches afterwards**.
  • Reason: (I was wondering about whether that's the right strategy but...) I saw it being done in create() and adopt() methods, among others. (Latest activity in this class; #2565249: Adopting a Table requires Cache Clear to show in Views has explicitly added a clearCache() to fix a bug.)
• Any necessary form submit function in data_ui.admin.inc which needs a cache clear and does not call a DataTable method which already clears the cache, calls DataTable::clearCache() by itself.
• Clearing the cache this way will always call drupal_get_schema(,TRUE) and views_invalidate_cache(), and optionally menu_rebuild(). The logic for this is arguable / a tradeoff. Here are my reasons:
  • menu_rebuild() is expensive, keeps being expensive when called repeatedly, and is only occasionally necessary: when the full table is added/removed. So it's optional.
  • drupal_get_schema(,TRUE) (note the first argument is inconsequential) is expensive, keeps being expensive when called repeatedly... but I am not adding any repeated calls. There are exactly two places where clearCaches() is called, where drupal_get_schema(,TRUE) isn't actually necessary: when adding or removing a Data table join. IMHO it's quite unlikely that the link() or unlink() methods will be called repeatedly, or called so often that the extra schema rebuild can be considered real overhead. Therefore I've leaned to not making this optional (thereby also limiting potential bugs stemming from not calling it by accident).
  • views_invalidate_cache() is expensive only once, also when it's called repeatedly. (Not when clearing the cache, but later when rebuilding caches.) It is now called more often than needed, e.g. when adding/removing an index/key. IMHO we don't have to care very much about that overhead, because all these functions aren't called that often... and we don't need to care about them being called in a loop. So I favor always calling it, for code consistency / limiting potential bugs.
• Note there are places where DataTable:: clearCaches() is called repeatedly in a loop: in data_ui_edit_form_submit(), on repeated addIndex() / dropIndex() calls. Also note that I am not introducing these; I'm simply replacing repeated drupal_get_schema() calls by repeated clearCaches() calls, which adds only low overhead (as per above).

Changes to achieve this:

• Introduced an optional argument to clearCaches(). (I would prefer making it FALSE by default, but that would introduce a behaviour change for external clearCaches() calls, so I made it TRUE.)
• In most cases, drupal_get_schema() calls are replaced with DataTable:: clearCaches(). Either to fix mild bugs (e.g. to remove a default view on drop() / to rebuild menus after creating a table), or just for code consistency which IMHO does not add much unnecessary overhead. (In the case of changing indexes/keys/etc.)
• In one case (data_ui_adopt_form_submit()), clearCaches() call was removed because it's done in the DataTable method already.
• data_ui_drop_form_submit() replaces the call to data_drop_table() with $table->drop() because that clears the cache.
• Introduced a new call to clearCaches() here and there, where it's apparently missing.
• One instance of changing clearCaches() to clearCaches(FALSE) where we didn't need the menu refresh.
• Removed some hacks from tests that should now be unnecessary. Added /** @var hints for convenience.
• Put a comment in DataTable::update() because that is called so often it shouldn't have any clearCaches() call. So that is up to its callers.

In summary, I believe that this is nice code unification, in exchange for a negligible amount of overhead (outside of the bugs that this patch fixes). Namely: three superfluous calls to drupal_get_schema() from DataTable::link() / DataTable::unlink() / data_ui_adjust_form_submit_update_schema().

It's also a good basis for further changes, IMHO. **This doesn't mean things need to stay this way. I just noticed that #618064: Remove cache clearing code from DataTable CRUD methods wants to go the opposite way and move the cache clearing out of the DataTable class... and I can see the sense in that, however

• That issue, unlike this patch, would be a considerable behaviour change.
• This patch does not make it harder to implement that. (It probably makes it easier.)

I briefly had a look at whether this would need a D8 version (even though I've never used Data for D8). But it seems that the DataTable class in D8 is less up to date (I spotted a D7 commit that wasn't applied to the D8 class) so maybe not.