Values overwritten when multiple aggregated fields exist on same data source

I was able to get the patch to mostly apply to the current version of 8.x-1.35 and it seemed to have resolved the issue.

Attaching the patch that I updated. There are some changes to the FieldHelper utility that didn't apply cleanly but it looks like everything is working. So I'm not sure if another issue might of incorporated some of those changes.

I think this is the correctly rebased version of the patch.
Anyways, we use MRs for development now, so I created an MR containing that patch.
Still needs tests.

Hello,
I am wondering whether it makes sense to add the item language for the missing fields. Otherwise there might be uses cases were the returned values correspond to the "original" translation.
For example (Solr scenario):

1. Search index: Indexing auto_aggregated_fulltext_field (body and tags). Where tags are terms referencing vocabulary tags (translatable terms).
2. Views configured fulltext for: auto_aggregated_fulltext_field.
3. Highlight processor: configured to show body and tags.
4. Searching in the translated language might not show the translated string/excerpt.

This:

      if ($missing_fields) {
        $this->extractFields($item->getOriginalObject(), $missing_fields);
        foreach ($missing_fields as $property_fields) {
          foreach ($property_fields as $field) {
            $item_values[$field->getFieldIdentifier()] = $field->getValues();
          }
        }
      }

Becomes that:

      if ($missing_fields) {
        $this->extractFields($item->getOriginalObject(), $missing_fields, $item->getLanguage());
        foreach ($missing_fields as $property_fields) {
          foreach ($property_fields as $field) {
            $item_values[$field->getFieldIdentifier()] = $field->getValues();
          }
        }
      }

Since i am working using a custom patch combining 🐛 Excerpt shows all fields even when only one field has the searchterm Active and this issue's patch, I wonder if using $item->getLanguage() for missing fields can break some code that I am not aware of.

I've added the logger service in the setUpMockContainer() along with a method in the HighlightTest.php for creating the needed FieldItemDataDefinition.

So now we have this function createFieldItemDataDefinition for testFieldExtraction.php, which avoids the error:

Drupal\Tests\search_api\Unit\Processor\HighlightTest::testFieldExtraction
Error: Call to a member function getClass() on null

I've added the missing field data definition for the 'entity:test2', avoiding the error:

Drupal\Tests\search_api\Unit\Processor\HighlightTest::testFieldExtraction
TypeError: Drupal\search_api\Utility\FieldsHelper::retrieveNestedProperty(): Argument #1 ($properties) must be of type array, null given, called in /var/www/html/web/modules/contrib/search_api/src/Item/Field.php on line 483

This is the configuration for the test:

        [
          'entity:test1',
          [
            'bar' => new DataDefinition(),
            'foobar' => new DataDefinition(),
            'bar' => $this->createFieldItemDataDefinition('text'),
            'foobar' => $this->createFieldItemDataDefinition('text'),
          ],
        ],
        [
          'entity:test2',
          [
            'foobar' => $this->createFieldItemDataDefinition('text'),
          ],
        ],

Creating the entity:test2, along with the the logger service, is needed and it will solve the error:

Drupal\Tests\search_api\Unit\Processor\HighlightTest::testFieldExtraction
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "logger.channel.search_api".

Which it is expected in the Highlight.php since we will have this configuration:

• field1: Nested property path (entity:test1/bar:foo) - extracts from original object
• field2: Simple property path (entity:test2/foobar) - different datasource
• field3: Processor-generated field (foo) - uses ProcessorProperty
• field4: Non-text field (baz, FALSE) - should be excluded from highlighting
• field5: Pre-existing field values (entity:test1/foobar) - already on item

And then:

      try {
        $property = $field->getDataDefinition();
        if ($property instanceof ConfigurablePropertyInterface) {
          $path .= '|' . $field_id;
        }
      }
      catch (SearchApiException $e) {
        $this->logException($e);
      }

I have "rebased" the branch with the latest changes (it was around ~57 commits behind).

The only test failing is the one related to the issue 🐛 Replace deprecated REQUIREMENT_ERROR constant in search_api_db_defaults_requirements() for Drupal 11+ compatibility Active .

And my only concern is whether or not we should add the item language to when $missing fields (see my previous comment):

if ($missing_fields) {
        $this->extractFields($item->getOriginalObject(), $missing_fields, $item->getLanguage());

The issue is ready for review as soon as we decide whether or not to include the $item->getLanguage(), which I think we should to it.

Thanks a lot for your work on this, your changes look good to me.
Did you also test the MR on an actual site or did you just fix the tests? If you can confirm this works for you I think this would be RTBC.

However, in any case we have to wait for 🐛 Replace deprecated REQUIREMENT_ERROR constant in search_api_db_defaults_requirements() for Drupal 11+ compatibility Active (and possible 🐛 Fix failing pipelines Active ?) to fix the pipelines.

If someone could confirm that the latest version of the MR works for them then I could merge this.

Sorry about the delay,
I've tested it in an actual site and it worked for me.

No worries, thanks for reporting back. And good to hear it works for you, too.
Merged.
Thanks again, everyone!