Issue #21 by bucefal91: Making sure the graphql API handles correctly…

[bucefal91]

This is PR that fixes https://github.com/drupal-graphql/graphql-search-api/issues/21

I am simply using is_null() instead of empty() which yields a bit stricter match against NULLness without affecting such values as (string) "" or (int) 0.

After applying this patch and clearing the cache on the same solr index my graphql now responds zeros as it should:

[duartegarin]

Hi @bucefal91 The problem with this patch is that I think it's possible for SearchAPI to yield an empty array in multivalue fields. In that case, the is_null won't yield true, which would be incorrect. I think we need to tweak the code to check for $field_values being empty, before we process the code to check the type and if it's single or multivalue. cc: @joaogarin

[bucefal91]

I see your concern. I wish I knew more of the internal guts here :) But I did spend now 1 hour trying to expose such edge case.

For example, I've created such document in my search index that has no value for a multi-value field. In this precise spot in SearchAPISearch.php the return from ::loadResponseDocument() was:

And on the outside I ran the following query:

As you can see, for the fields playlist_field_transcript (multivalue long text), playlist_video_name (multivalue string), playlist_field_audience (multivalue integer) it did handle the job pretty nicely.

I am not trying to say my patch does not hurt this side of empty arrays, but I do say after having tried to reproduce the possible error I failed.

[duartegarin]

Hi @bucefal91 , This was an interesting one as it made me realise some code inefficiencies. The reason that is_null works, is that we are basing the cardinality on the count of the field values. Since it will only be considered multivalue in that sense if it has data it never arrives as an empty array. I need to investigate this further, but by all means use this patch meanwhile, I won't release anything until this is properly sorted.

[bucefal91]

No problem. By no means I urge the acceptance/commit of this patch. Take your time. I do appreciate you being responsive here and at least confirming you do not see any direct pitfalls with the patch (hence I have higher degree of confidence that it works and it works OK). So far in our specific codebase we haven't discovered side effects with this patch.

Good job on this module. I really like it :) And it saved us from the need to code API for search ourselves :)

[duartegarin]

Closing this in favour of PR #23 seem #21 for details.