- Issue created by @a.dmitriiev
- 🇩🇪Germany a.dmitriiev
I think the following methods from Index entity class should be exposed as config actions:
setOption
(without plural support, as there is alreadysetOptions
method in the class)setOptions
removeDatasource
as only the id is needed for argumentremoveProcessor
renameField
removeField
it would be also nice to have the following methods exposed, but some code adjustments will be needed:
addDatasource
, but at the moment the argument is of type Datasource and it can't be used in recipe. Maybe alternatively there could be a methodaddDatasourceByIdAndConfig
that will have datasource id and its config as arguments.setTracker
has and object as argument so it can't be used as action method, but maybe having a methodsetTrackerByIdAndConfig
with tracker plugin id and config as arguments would make sense- The same for
setServer
it makes sense to createsetServerById
? addProcessor
has object as argument, but maybe new methodaddProcessorByIdAndConfig
would be ok to add as welladdField
has Field object as argument, but maybe new methodaddFieldByFieldConfig
would be ok to add
Methods that are not in Index class yet, but might be useful:
setServer
setReadOnly
Methods for Server entity:
setBackendConfig
Missing methods on Server entity, that could be useful:
setBackend
- as setBackendConfig method assumes that backend property is already set, maybe it is needed to have backend set method as well. - 🇩🇪Germany a.dmitriiev
I have prepared the first MR for recipe integration https://www.drupal.org/project/search_api/issues/3484304 ✨ Expose Index and Server methods as config actions Active . It exposes only already existing methods as config actions to start this feature rolling.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I have prepared the first MR for recipe integration https://www.drupal.org/project/search_api/issues/3484304 ✨ Expose Index and Server methods as config actions Active . It exposes only already existing methods as config actions to start this feature rolling.
This is a good and easy first step, I think that one is ok.
- addDatasource, but at the moment the argument is of type Datasource and it can't be used in recipe. Maybe alternatively there could be a method addDatasourceByIdAndConfig that will have datasource id and its config as arguments.
- setTracker has and object as argument so it can't be used as action method, but maybe having a method setTrackerByIdAndConfig with tracker plugin id and config as arguments would make sense
- The same for setServer it makes sense to create setServerById?
- addProcessor has object as argument, but maybe new method addProcessorByIdAndConfig would be ok to add as well
- addField has Field object as argument, but maybe new method addFieldByFieldConfig would be ok to add
For datasource, and server I think adding a new method is a good idea.
Processors and fields are more difficult, because their configuration can be more extensive, but I guess we can go through the same way here as well?I think tracker is probably not needed, since for 99% the basic tracker is used. So I'm not sure how I feel about creating this new method, however if we already do this same trick for the other 4 I guess it's not too bad?
- 🇦🇹Austria drunken monkey Vienna, Austria
Great initiative, thanks! I already merged your first MR, just making existing methods available seems like a low-hanging fruit.
My thoughts on the rest of your suggestions:
setServerById()
: I would just call thatsetServerId()
maybe? Would seem like a natural extension from the existinggetServerId()
. It’s just a bit unfortunate thatgetServerInstance()
andsetServer()
don’t follow this sensible pattern.addDatasourceByIdAndConfig()
: We already added a separate config action plugin for that in ✨ Create a config action to add a data source to an index Fixed . However, if we add new methods for the other plugin types, doing it for datasources would also make sense for the sake of consistency – not sure how to handle that, we probably don’t want to offer two config actions for the exact same thing.setTrackerByIdAndConfig()
,addProcessorByIdAndConfig()
,addFieldByFieldConfig
: I admit I first balked a bit at thes suggestions, as I dislike such verbose method names and the methods would pretty much duplicate existing ones. I therefore wanted to suggest instead adding dedicated config action plugins to implement this functionaliy, as we already have for datasources (see above).
However, on the other hand it does seem a bit strange to have something available to site admins but not to developers. Also, I suspect the DX of those new methods would be better anyways than of the existing ones – I would now say we probably went the wrong way there when first writing the D8 version of the module.
So, I guess I’m now on board with just adding those methods. I can also imagine our own code subsequently drifting towards using those new methods. They certainly seem handier in most situations. (Though the lack of dependency injection for entities continues to be vexing in this regard.)
As mentioned above, though, we should think about what to do with datasources. We will probably want to add theaddDatasourceByIdAndConfig()
method in any case, but maybe just not make it available as a config action?setBackend()
: This would be a major architectural change as a server’s backend plugin is currently immutable. I would therefore need a very good argument why this would be helpful/needed before considering it.setReadOnly()
: Yes, good idea. No clue why this is currently missing.
Note that these should all also be added to the interface, in my opinion, which means you’ll have to add them to the
UnsavedIndexConfiguration
class, too. (Just as a tip, as I regularly forget that as well.)Methods that are not in Index class yet, but might be useful:
setServer
setReadOnly
setServer
seems like a typo, as that method already exists (and is listed by you a few lines above). Do you know what you meant? - 🇭🇺Hungary mxr576 Hungary
Two additional action suggestions:
- Index items - with optional data source filtering. Also useful when default content import doesn't trigger indexing automatically - haven't checked to root cause but experienced this when I wrote a test for a recipe
- Assign index to search backend - for recipes that bundle indexes without backends. Should filter by server type (e.g., Solr) with fallback option to prevent exception when a match not found.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I agree with the second one; that sounds useful. The first one however (index items) seems like it shouldn't be happening at all?
- 🇭🇺Hungary mxr576 Hungary
Well, I guess the answer is "it depends". Whoever uses that action should be awareof consequences. However without that it is complicated to set up an immediately usable demo env with recipes, where waiting for cron to index items may not be an option.