Split up Utility into one or more services

I spoke with Andrei. His suggestion is to first split out all the fields-related functionality into a service and then review what's left.
One issue there would be that the methods for creating a query (and others) could still not be switched out. So probably we'd then need a second service for those other methods that someone might conceivably want to override.

While we're at a major overhaul of that class, we should also pick one of self:: or static:: for inner-class calls and stick with that – currently, it's just arbitrary, and we should at least be arbitrary and consistent. (Probably static is better, in case anyone really does extend the class, however improbable. It's also more often used currently (14 vs. 6).)

To support the (theoretical) use case of #2230907: Split up Utility into one or more services, or similar ones that might come up, making this a service (or a collection of several services) seems to make the most sense.
(In which case the self/static question becomes moot anyways.)

For those methods for which overriding them has no real use case, instead of leaving them in Utility splitting them into traits and including those where needed would also be a variant. (Suggestion coming from Nick.)
Would have the advantage of separating different functionality more cleanly. Not completely sure it's worth the additional code changes, though.

In any case, we should probably wait with this until less patches are in the queue, since this will most likely break all of them.

OK, so I've finally tackled this, and the first idea I had was to keep Utility with all its methods for now and just mark them deprecated. Then, in all new code, we can just use the new services and thus we should have a lot less trouble transitioning at some point during Beta (i.e., removing the methods from Utility).

Then, I saw that about 90% of Utility is indeed fields-related. I split those into their own service, which I called FieldsHelper (better naming suggestions welcome). It also contains the three data type-related methods (isTextType(), getFieldTypeMapping(), getDataTypeFallbackMapping()) – if you think, we can also create a separate service for these.

The createQuery() method I moved/copied to the ResultsCache service – a pretty good fit regarding considering the functionality (and that that method actually retrieved the results cache service), even though the naming is a bit weird now. Usually, people will use Index::query() anyways, though, so it shouldn't matter much. On the other hand, we can also rename it to something like SearchHelper or QueryHelper, if we decide that would be better. (Again, better naming suggestions welcome. *Service? *Manager?)

The processIndexTasks() I just moved to the IndexTaskManager service (keeping it static, though, by necessity (as far as I'm aware) – still, makes more sense there).

This leaves five methods, which I think we can just keep as static Utility methods: createTextToken(), deepCopy(), createCombinedId(), splitCombinedId() and splitPropertyPath().
We could also split them into one or more traits, but I don't really see the advantage of that (some of these are pretty ubiquitous, and having traits with a single method for the others would be also weird). But all is up for discussion.

...the first idea I had was to keep Utility with all its methods for now and just mark them deprecated. Then, in all new code, we can just use the new services and thus we should have a lot less trouble transitioning at some point during Beta

I agree, that makes sense.

copy from src/Utility.php
copy to src/Item/FieldsHelper.php

I don't understand the move into Item, I'd suggest we move this into src/Utility/FieldsHelper.php

It also contains the three data type-related methods (isTextType(), getFieldTypeMapping(), getDataTypeFallbackMapping()) – if you think, we can also create a separate service for these.

I think we should, I suggest DataTypeHelper

The createQuery() method I moved/copied to the ResultsCache service – a pretty good fit regarding considering the functionality (and that that method actually retrieved the results cache service), even though the naming is a bit weird now

The naming is very weird, let's move this into the Query class and rename the method to ::fromResultCache?

The processIndexTasks() I just moved to the IndexTaskManager service (keeping it static, though, by necessity (as far as I'm aware) – still, makes more sense there).

I agree, that makes sense.

This leaves five methods, which I think we can just keep as static Utility methods: createTextToken(), deepCopy(), createCombinedId(), splitCombinedId() and splitPropertyPath().

Sure, let's move those to src/Utility/Utility.php?

I also don't understand why you created FieldsHelperInterface, there won't be any alternative implementations of this and not everything needs an interface. So let's just remove that.

Oh, of course, this once again complicates the unit tests. A lot.
(Also, switching the new service and the TestItemsTrait to camelCase for everything.)

Back to NW for #9

Ah, sorry, cross-posted with you there.

I don't understand the move into Item, I'd suggest we move this into src/Utility/FieldsHelper.php

Well, the functionality belong to items and fields, so that namespace/folder seemed natural. Utility would have been an option, too, but I didn't want a namespace with just a single class (or class plus interface). But if we now get more classes there, Utility is fine.

I think we should, I suggest DataTypeHelper

OK.

The naming is very weird, let's move this into the Query class and rename the method to ::fromResultCache?

That would completely defeat the purpose of moving it to a service in the first place – i.e., being able to switch the query class (see #1). So if the naming is too weird, I'd rather suggest renaming the service (and, possibly, the existing methods). Maybe SearchHelper, staying with the nomenclature in this issue? Optionally, we could also move it to Utility.

Sure, let's move those to src/Utility/Utility.php?

OK.

I also don't understand why you created FieldsHelperInterface, there won't be any alternative implementations of this and not everything needs an interface. So let's just remove that.

If there wouldn't be (the possibility of) alternative implementations, having it as a service would be pointless.
Also, we pass it to some constructors/methods and I dislike type-hinting with class names. When in doubt, I like to err on the side of too many interfaces. It doesn't really do any harm, does it?

The naming is very weird, let's move this into the Query class and rename the method to ::fromResultCache?

That would completely defeat the purpose of moving it to a service in the first place – i.e., being able to switch the query class (see #1). So if the naming is too weird, I'd rather suggest renaming the service (and, possibly, the existing methods). Maybe SearchHelper, staying with the nomenclature in this issue? Optionally, we could also move it to Utility.

Sure, SearchHelper makes sense - maybe QueryHelper?
I figured ::fromResultCache was a good name to keep in line with the URL object in drupal core. That has ::fromRoute, ::fromUserInput, ... and I figured this would make it follow that very closely. You can still switch the query class and have it extend the original query class, as long as ::fromResultCache returns new static, I don't see any issues with that. If you disagree, my vote goes to QueryHelper.

I also don't understand why you created FieldsHelperInterface, there won't be any alternative implementations of this and not everything needs an interface. So let's just remove that.

If there wouldn't be (the possibility of) alternative implementations, having it as a service would be pointless.
Also, we pass it to some constructors/methods and I dislike type-hinting with class names. When in doubt, I like to err on the side of too many interfaces. It doesn't really do any harm, does it?

I don't think a service is pointless even if we don't see alternative implementations, it's easier to get a class like that from the container or have it injected with all it's dependencies.
It doesn't really do any harm, other than making the code a little bit more complex.
I understand the argument about not wanting to typehint on classnames though, so I guess keeping the interface doesn't hurt.

You can still switch the query class and have it extend the original query class, as long as ::fromResultCache returns new static, I don't see any issues with that. If you disagree, my vote goes to QueryHelper.

You can also just create a new class implementing QueryInterface and use its constructor. Using a different query class in your own code is never a problem – the problem is being able to replace the query class in other people's code (such as the code contained in the Search API itself). It's probably rather a niche use case, but if we can support it without much hassle, I'd still call that a plus.

So, I went with QueryHelper and moved it to src/Utility along with its interface.

I don't think a service is pointless even if we don't see alternative implementations, it's easier to get a class like that from the container or have it injected with all it's dependencies.

Utility didn't have any dependencies, and just using the class name and doing a static call is certainly a lot easier than any way of obtaining the service. The main purpose of making it a service is not ease of use, in my opinion, but making as much code as possible overridable (either by other modules, or just for testing purposes).

I think your branch is not rebased with the latest changes, there seem to be unrelated changes in the patch.

Oh, damn, you're right! Thanks a lot for noticing that, that would have been pretty bad!

1. +++ b/tests/src/Unit/Plugin/Processor/TestItemsTrait.php
   @@ -32,29 +36,29 @@
   -   * @param string $field_type
   +   * @param string $fieldType
   ...
   -   * @param mixed $field_value
   +   * @param mixed $fieldValue
   ...
   -   * @param string $field_id
   +   * @param string $fieldId
   

   These changes are unrelated, but I like them. Let's keep those in.

2. +++ b/tests/src/Unit/Plugin/Processor/TestItemsTrait.php
   @@ -82,7 +86,7 @@ public function createItems(IndexInterface $index, $count, array $fields, Comple
   -      $item = Utility::createItem($index, $item_id);
   +      $item = new Item($index, $item_id);
   

   I like this change, is it possible to do this in other places as well?

Those are the only things I noticed - putting this back to NW for .2.

Ah, needed a (small) re-roll anyways …
I don't really know why I made that change, but sure, we can also do that in the rest of the test.

Awesome, if the tests agree with this, so do I.

OK, probably just using Utility::createField() there is the easier choice.

Committed.
Thanks a lot for reviewing!