Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Context Is Lost On Aggregation #286

Closed
kiler129 opened this issue Feb 13, 2019 · 2 comments · Fixed by #288
Closed

[Bug] Context Is Lost On Aggregation #286

kiler129 opened this issue Feb 13, 2019 · 2 comments · Fixed by #288
Labels

Comments

@kiler129
Copy link
Contributor

  • Symfony version: 4.3.2
  • Algolia Search Bundle version: master branch
  • Algolia Client Version: irrelevant
  • Language Version: irrelevant

Description

While working on #283 implementation I discovered sometimes context is lost. I was able to track it down to these lines:

public function normalize(NormalizerInterface $normalizer, $format = null, array $context = [])
{
return array_merge(['objectID' => $this->objectID], $normalizer->normalize($this->entity));
}

Is it intentional that format and other options (e.g. serialization groups) are ignored on this normalize() call? IMHO the code should NOT ignore them, since it will break custom normalizers.

@nunomaduro
Copy link
Contributor

Thanks for this @kiler129, could this add some side-effects on people already using the search bundle (with this bug) ?

@nunomaduro nunomaduro added the Bug label Feb 14, 2019
@kiler129
Copy link
Contributor Author

@nunomaduro Basically no custom normalizers configured to support Algolia-related serialization will be invoked.

Imagine an aggregator with such definition:

    public static function getEntities()
    {
        return [
            Post::class,
            Article::class
        ];
    }

Then you have such normalizer:

class RandomNormalizer implements NormalizerInterface
{
    public function normalize($object, $format = null, array $context = [])
    {
        //... some logic
    }

    public function supportsNormalization($data, $format = null)
    {
        dump('Object type: ' . \get_class($data));
        dump('Format: ' . $format);
        dump('--');

        return $format === Searchable::NORMALIZATION_FORMAT && $data instanceof Post;
    }
}

WITHOUT my fix the condition in supportsNormalization() will never be hit. The output of import will be similar to this:

"Object type: App\Entity\LibraryThing"
"Format: searchableArray"
"--"
"Object type: App\Entity\Post"
"Format: "
"--"
Indexed 1 / 1 App\Entity\Post entities into dev_library index
"Object type: App\Entity\LibraryThing"
"Format: searchableArray"
"--"
"Object type: App\Entity\Article"
"Format: "
"--"
Indexed 1 / 1 App\Entity\Article entities into dev_library index
Done!

WITH my fix:

$ bin/console search:import
"Object type: App\Entity\LibraryThing"
"Format: searchableArray"
"--"
"Object type: App\Entity\Post"
"Format: searchableArray"
"--"
Indexed 1 / 1 App\Entity\Post entities into dev_library index
"Object type: App\Entity\LibraryThing"
"Format: searchableArray"
"--"
"Object type: App\Entity\Article"
"Format: searchableArray"
"--"
Indexed 1 / 1 App\Entity\Article entities into dev_library index
Done!

...and this is just $format, the same applies to any data in the $context.

Essentially aggregators, without the fix, makes impossible to use any custom normalizers only for Algolia on aggregated entities (since format & context is only passed for aggregator itself, not for inner entities).

nunomaduro added a commit that referenced this issue Feb 18, 2019
#286: Carry over normalization context on aggregation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants