move date handling to a processor, so date facets can use existing widgets

I was running trough the list of issues earlier noticed that issue and was about to close (won't fix) because I figured that adding duplicates for all existing widgets would be too much work / maintenance. I agree that this approach is a more solid way to fix this. The only thing that I'm a little bit worried about is the getQueryType method is on the widgetInterface.

We might have to move that to the facet-entity and make it configurable trough the UI.

If we do that - it's an API break and it would mean that this issue turns into a beta blocker.

Please remember that we still need to provide a way for widget to require / prefer specific query types. This is needed for range widgets, they should work for the range query type.

I think the WidgetInterface::isPropertyRequired can be used for that though.

1. Move most of the code from DateBasicWidget to a new DateItemProcessor plugin (with changes to the form due to #2896237: processor plugins aren't using plugin configuration correctly...)
2. Add getQueryType() to the processor interface and base class. By default, this returns NULL.
3. Change WidgetPluginBase::getQueryType() to return NULL to mean 'use the default query type'
4. Change Facet::getQueryType() so it also calls the processors's implementations of getQueryType(). If the widget and all processors return NULL, the 'string' query type is used. If one or more return the same type, use that. If there is a disagreement between any of the processors and the widget, throw an exception (for now -- see #2895369: prevent unsuitable processors being added to facets).
5. Change query type plugins so instead of calling out to get configuration options (eg SearchApiDate::getGranularity(), getDisplayRelative(), etc), it's passed in to them by code that create the instance. This is necessary so that widgets and processors can have their query options merged. That could be done in DefaultFacetManager::alterQuery(), but it makes more sense for QueryTypePluginManager to override createInstance().

#2896237 is fixed, so that shouldn't be a problem anymore.

1. Yes, that makes sense. Might have to move some of the tests as well (if we have unit-test coverage for that widget, I think we do)
2. Sure, sounds reasonable.
3. Also sounds reasonable.
4. This makes a lot of sense. Throwing an exception there sounds like a solid idea. An uncaught exception there should notify developers that their config is wrong. Until we get to the followup this sounds perfect.
5. In the plugin manager is a good idea, might be a good idea to not stuff too much extra stuff into the FacetManager, that class already has the highest complexity of everything we have.

Overall this sounds like a very solid approach to fix this problem.
This should also make the work going on in #2892620: As a sitebuilder I want a map to be exposed as a facet filter so I can narrow down results by dragging and zooming the map simpler. So I'm all for that!

Should we also tackle the same issues for the ranges here as well? We probably should do this all in one pass.

This is a start on all the required work. Needs test as well as .5 discussed in #7 to get this to actually work.

Back to needs work, sad that not a lot of our tests broke. Means that we really have to write more tests to make sure that this works as expected.

Also, should we write an upgrade path to change the widgets to links for everything that used datebasic?

I tested this and it works well. Leaving code review for someone more familiar with the module.

Doesn't work anymore now that we committed #2899947: Date list breaks when using exclude. Doesn't work yet with the patching I did to try and fix it.

Should be green now, with correct config object.

Just throwing some things here as I work through the code. Core search hasn't been converted to 'type' base, but still field based. It certainly needs to always have a string.

--- a/modules/core_search_facets/src/Plugin/facets/facet_source/CoreNodeSearchFacetSource.php
+++ b/modules/core_search_facets/src/Plugin/facets/facet_source/CoreNodeSearchFacetSource.php
@@ -182,21 +182,14 @@ class CoreNodeSearchFacetSource extends FacetSourcePluginBase implements CoreSea
    */
   protected function getQueryTypesForFieldType($field_type) {
     $query_types = [];
+    $query_types['string'] = 'core_node_search_string';
     switch ($field_type) {
-      case 'type':
-      case 'uid':
-      case 'langcode':
-      case 'integer':
-      case 'entity_reference':
-        $query_types['string'] = 'core_node_search_string';
-        break;
-
       case 'created':
       case 'changed':
         $query_types['date'] = 'core_node_search_date';
         break;
-
     }
+    // In the future can also have cases for range.
 
     return $query_types;
   }

Equally because it doesn't support range, yet it's easy to configure to do so without warnings and if a query type isn't supported an exception InvalidQueryTypeException is thrown from Facet::pickQueryType() it should probably be caught and watchdog/set message; as it stands the error isn't so helpful;
[edit] and I notice after conversation it's not what happens with mis-configurations where the widget doesn't match the processor: 'Invalid query type combination in widget / processors. Widget: %s, Processors: %s.'

Hard coding the exception for date_item into QueryTypePluginManager::createInstance and moving the configuration there makes the query both reliant on the processor, and makes it difficult for other processor combinations to pass on different configuration.

Sticking with the granularity option which is clearly a query level option. Easy option the facet is available so in getGranularity of SearchApiDate: knowing its required processor it can just retrieve something like $this->facet->getProcessorConfigs()['date_item']['settings']['granularity']. Equally a processor would be needed for other numbers (it's not yet updated yet?): SearchApiGranular.

Could something like granularity come from more than one processor? Probably. What then? Make explicit particular processor requirements?

Beyond those two I think this does make it easier to reuse queries.

Also if I'm thinking this through correctly? For my forever plan to build native solr range facets (and probably better sql ones) it would be possible to create a (required) processor; a query using it; and then not have to recreate the widgets to show the output which would by that point be the same as the stuff produced by those horrid loops.

Just reading through again:

--- a/config/schema/facets.processor.schema.yml
+++ b/config/schema/facets.processor.schema.yml
@@ -98,3 +98,17 @@ plugin.plugin_configuration.facets_processor.hide_active_items_processor:
 
 plugin.plugin_configuration.facets_processor.list_item:
   type: config_object
+
+plugin.plugin_configuration.facets_processor.date_item:
+  type: mapping
+  label: 'Boolean processor'
+  mapping:

Probably shouldn't be the 'Boolean processor' again.

Still don't like the selection method if() if() logic, still can' t think of a better way. Will try out basically extending it. If that's good then I think this should get in sooner rather than later :-)

Crediting ekes for the review work on this patch

Fixed the issue discovered in #26

Tested and works as expected;
Very small nitpick in patch

Needs upgrade path and reroll

Reroll attached for tests.
I'll start working on upgrade path now

Upgrade path attached

Refactored the config keys to match more with what we are actually using them for ;)
Since we are doing an upgrade path, we can do this at once, as we need to migrate that specific config anyway.

+++ b/facets.install
@@ -101,17 +101,24 @@ function facets_update_8003() {
+        'granularity' => $widget['config']['granularity'],

Since we're renaming the config anyway, should we rename this to date_granularity? This doesn't just apply to this piece of the code but everywhere we're doing granularity things.

eh, I have not really an opinion about that. Both are fine for me. The other renames I did was because they were used wrong, thus making it not clear at all. This one makes sense already ;)

In that case, this looks solid!

Needs a reroll.

Reroll attached.

go testbot go.

Reroll

Clashing test name. C/P error
@borisson_ A beer will do ;)

Date handling is now better, fixed, thanks so much everyone!