Aggregated text plugin setting field value without datatype

I'm guessing the AggregatedFields plugin addFieldValues() method shouldn't be wrapping the value in an array and using the FieldInterface::setValues() method, but rather adding the string and using FieldInterface::addValue() to allow it to handle processing the value with the correct datatype plugin.

Ala

Pop to review, not sure what tests would be covering this one.

Just that one then.

+++ b/src/Plugin/search_api/processor/AggregatedFields.php
@@ -74,35 +74,43 @@ class AggregatedFields extends ProcessorPluginBase {
+        $text_value = '';

The default should be NULL, in which case you wouldn't add a value at the end.

Or, instead of changing all this, you could leave it as-is and do a foreach … ->addValue() afterwards.

For tests, see #2752963: rendered_item values overridden by processors – just add them to the existing AggregatedFieldsTest and check that all field values are actually passed through the data type plugins (probably by using a mockup data type plugin). (Or, just check that all fields that are of type "Fulltext" have only TextValueInterface objects as values. You can even set all fields to "Fulltext" for that, it shouldn't really matter in any other way.)

This is how I'd do it. Tests/reviews welcome!

+++ b/tests/src/Unit/Plugin/Processor/AggregatedFieldsTest.php
@@ -84,6 +91,28 @@ protected function setUp() {
+    /** @var \PHPUnit_Framework_MockObject_MockObject $data_type_manager */

It feels weird to hint agains the mockobject, if we add any kind of mock here, it should be plugin manager's one.

I did a search in core and it looks like there's a bunch of instances where there's being mocked on * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\user\UserInterface, so we can do something similar if you want to keep the mock

Otherwise this looks great, so apart from that I feel this is RTBC. Setting to NW for that small change though.

It feels weird to hint agains the mockobject, if we add any kind of mock here, it should be plugin manager's one.

(I guess you meant "any kind of type hint"?)
The hint for the mock object class is there since we just need the mock object methods. The manager class would be inferred by a properly configured IDE anyways. In this method, we don't need any of its methods, though, so I just type-hinted to the class whose methods are needed.

But if you find that confusing, it's no problem to add the second type hint (I did the same a few times in our code already, too).

Attached a re-roll with the second type hint.

Yes, thanks for being so kind to listen to my nitpickings :)

Yes, thanks for being so kind to listen to my nitpickings :)

I should be the one thanking you, for your thorough reviews! Usually you're right about such things, so no need to thank me for listening to them.

Anyways, committed. Thanks again for your work here, everyone!

works like a charm now