Call to a member function getSetting() on null in TranslateEntityProcessor

Setting to NR for testbot

Reuploading the patch to trigger the testbot

This patch solved the issue I was having for taxonomy terms not being able to determine the base field type.

Ok so it was failing for taxonomy term IDs but if you used the top level term value it was as is so I put the two together to solve the problem I was having.

That looks like a good solution, but because the unit test here is tightly tied to the implementation, we'd probably have to rewrite that one.

I'm inclined to actually rewrite it into a kernel test because we use so much of the entity api in this plugin. However I guess we can just fix the unit test and add a kernel test in a followup.

According to bot's failed messages, the patch doesn't solve the original problem. The problem is $field_config = $field_storage->load($entity_id); can be null. Then it gives error when code tries to call function 'getSetting()' on NULL.

So I got the same issue with one facet. The problem is the difference between the indexed field machine_name as compared to the original field machine_name attached to the entity the logic is indexing.

In my case, the error can be reproduced as following:

1. Create a job node type, and it has a field called 'Category' using machine name 'field_job_category'. This field is a taxonomy reference field.
2. Create an index for this node type. The index includes above category field. However, the machine name for this indexed category field is 'field_job_categories'(just use a different machine name as example) at http://example.dev/admin/config/search/search-api/index/jobs/fields.
3. Create a facet based on this category field and add the translate label processor on this facet.
4. Visit the facet display page, and the error throws.

So I have three solutions for this problem:

1). Create another translate processor for taxonomy term. The difference between this taxonomy term translate processor and the origin entity label translate processor is this one ignores the part of searching the entity type. See the diff https://www.drupal.org/files/issues/translater_processor.diff.

2). Recreate the indexed category field machine name, and make sure it's the same as the content type's field machine name. So $field_storage->load($entity_id); can have the correct $entity_id value 'node:field_job_category'. Reindex and recreate a new facet based on this changed field. The issue will be solved.

3). Solution 2) might not be a good idea when two indexed fields use the same content field from node type. For example, I have another indexed field 'job categories' field (at http://example.dev/admin/config/search/search-api/index/jobs/fields) which aggregates the 'job category' field and 'job sub-category' field(at http://example.dev/admin/structure/types/manage/job/fields). Then it won't be okay for the indexed field 'job categories' to use the same machine name as indexed field 'job category'.

So the third solution here is to update the origin entity label translate processor. When this processor is selected, the back-end will try to load the entity type, and if the entity type can't be retrieved as per the indexed field's machine name, it will show an entity type selection field and ask user to manually select the entity type for translation.

This is a refactored patch that abstracts the entity type discovery to a separate method and changes the way the previous patch worked slightly rather than converting the object to an array. This still needs tests, I haven't done anything with that, however this does fix the issue for me.

Additionally, @smiletrl, $field_config = $field_storage->load($entity_id); is no longer present in the last couple of patches I don't believe, so it returning NULL should not be a concern anymore AFAIK.

Let's see how the bot feels about #18.

This is a refactored patch that abstracts the entity type discovery to a separate method and changes the way the previous patch worked slightly rather than converting the object to an array. This still needs tests, I haven't done anything with that, however this does fix the issue for me.

I like that this is moved to a separate method, that makes it easier to read. This does need an additional test and the unit test needs to be fixed as well.

Here's a version of the same patch re-rolled to be compatible with the changes made over at: https://www.drupal.org/node/2772745

It still needs tests however. And note that this patch will not apply without the other one (https://www.drupal.org/node/2772745) being applied first.

@bmcclure, ur patch refactored after applying the changes in 2772745 is missing the line
use Drupal\Core\Field\FieldItemInterface;

Thanks,
Sukanya

Thanks for finding that issue! Here's a new patch with the use statement added.

Added support for SearchApiViews.

@Jeroen_005: I don't think SearchApiViews exists anymore in the latest Facets releases, I believe SearchApiDisplay is used in all cases now for Search API. However, I could be wrong about that as I haven't spent much time in Search API's code yet.

Hmz, seems to be correct, I was running on a fixed search_api commit (223094b). Upgrading search_api to the latest dev commit (cf740ca) solved my issue :-)

I can confirm that updating to search_api commit cf740ca and applying patch #26 fixed the issue for me.

Reupload of #23

@borisson_: I think #23 was missing a use statement that #25 adds back in. Not sure if that's causing the failure, but may be an issue.

It seems the latest patch does not solve this issue.
The test results are the same with the issue

PHP Fatal error: Call to a member function getSetting() on a non-object in /d8_path/modules/contrib/facets/src/Plugin/facets/processor/TranslateEntityProcessor.php on line 133

I also confirm the last comment about missing use

+use Drupal\Core\Field\FieldItemInterface;

But this is not solving the issue.
Maybe there is something need to be updated in the Tests or there something else missing.

p.s.: would it be more than nice to have interdiff files for the new patches.

I don't get that error anymore using the patch. However, that error would seem to indicate that $data_definition = $source->getIndex()->getField($facet->getFieldIdentifier())->getDataDefinition(); might be null and we should probably check that before trying to call getSetting() on the result.

>might be null and we should probably check that before trying to call getSetting() on the result.
This may not be the case as long as the configuration is correct. That should be enforced by CMI, so need to do this check imo.

Here is slightly different but very similar patch from #2851732: Entity label facet processor fatals if search api fields are renamed - this works fine for me.

The default to "node" seems quite dumb though, considered also nodes will have term references. Generally, I think this could be improved via #2851851: Facet source plugins should provide metadata for facet fields.

I think I agree with the approach suggested by @fago, this will make our api more clear and will remove the guesswork we're doing in this (and other) plugins.

I've been hesitant on this issue because the latest patches looked convoluted, but I didn't have a better path. The patch in #36 looks a lot cleaner.
I'll be at the belgian drupal user group meetup tonight and I see that StryKaizer, nick_vh and swentel will be there as well. I'll try to discuss this patch + #2851851: Facet source plugins should provide metadata for facet fields with them.

Thanks for the awesome work here @fago!

Thanks for the new patch! #36 does not seem to fix this issue for me, while the previous patches did. However, perhaps I have a configuration issue as fago mentioned, though unsure how that would happen or what should have been in place to guard against it. Will #2851851 somehow help to ensure this?

Fully agree the default of "node" isn't ideal; there should always be an entity type specified, though that not being the case was one of the main reasons for this patch it seemed.

Had to go back to a previous patch to get things fully working for me, but will try #36 again when I've got more time to see if I can find out what the difference is.

The patch in #36 works on a clean install for me, I think (though I'm not fully reproducing the test case from my other site). So is the real issue whatever has caused the configuration to be invalid for the cases where there are exceptions due to null values?

@fago: as promised, I discussed this with @swentel and @strykaizer and they both agree that your solution is a good improvement.

So next steps include:
- Fix the testfails here by changing the mocks around. Possibly easier to change to a kerneltest, not sure.
- Discuss and implement #2851851: Facet source plugins should provide metadata for facet fields
- Open followup issues for all processors that use the entity api to use the new metadata.
- Win!?

I think that's it?

I think I finally got #36 working without an unhandled exception on my dev site. I'm not entirely sure what fixed the broken configuration from before. But I can't reproduce my problem from yesterday anymore. Thanks again for the much-simplified solution!

I ran into this problem a couple of days ago, but quickly realized that the altered indexed field machine name was to blame.

I had some free time, so I reproduced the error then tested #36. After applying the patch I no longer get the fatal error and translate_entity processor works with no problem on a field with a different index machine name. The fix in #36 is RTBC +1 from me.

Thanks for confirming that @dcam, @bmcclure. All we need to do now is get the unit test to pass again.

I'm probably not going to be able to work on this before drupalcamp northern lights but it's good to have confirmed that this patch does fix the problem.

I was on the plane, and I only had this issue as an up-to-date branch so I had 3+hours to figure out the testfails and improve the overall readability of that unit test. Work attached. All tests should be green. When the bot comes back green we can commit this.

Committed the issue, thanks for all your work here everyone. Also huge thanks to @fago for making the patch so much simpler.

After updating to the latest dev version I got:

( ! ) Fatal error: Call to undefined method Drupal\search_api\Plugin\search_api\processor\Property\AggregatedFieldProperty::getPropertyDefinition() in /modules/contrib/facets/src/Plugin/facets/processor/TranslateEntityProcessor.php on line 107

I'm using the latest dev version of Search API and Facets.
Does anyone encounter the same issue?

Re. issue reported in #48 I'm not sure how TranslateEntityProcessor would work with aggregated fields because they might contain different entity types.

I faced this issue as well and my workaround was creating a custom processor, however I believe the fatal error above should be prevented.

I had had the need of aggregating several taxonomy fields, I therefore created a processor to transform taxonomy term ids into term names.

Attaching my processor with the hope it may help.
Any thoughts?

Your processor is exactly what should be need to do this. The fatal error should be replaced with a new hard exception, there's a new exception added in #2851851: Facet source plugins should provide metadata for facet fields. That should make it more clear what's going on.

Just removing exeptions is not something we want to do. Adding specific errors with specific messages makes it a lot easier to find what exactly is going on.