Rewrite subscriber to Listener, and add lazy

[cryptiklemur]

Due to how doctrine is written, in order for doctrine to see what events are subscribed to, the class needs to be instantiated. The Subscriber defined in this library depends on the client, which depends on the ClientContext. This ends up requiring that ALGOLIA_APP_ID and ALGOLIA_API_KEY are defined and correct in any and all environments, where they may not be necessary.

My specific use case is building a docker image. I should not have those variables in my docker image, but rather in the docker container as environment variables. Due to how Doctrine does this, I am unable to run a ./bin/console cache:clear as it runs the above code.

According to @weaverryan, you should be able to change to a Doctrine EventListener, and add lazy: true to the listener's tags, and this problem will go away.

symfony/symfony#27661

[julienbourdeau]

I will try it! I agree it should be lazy, this env variable errors is pretty annoying :D
Thanks for opening this issue.

[julienbourdeau]

So I looked at it back then, and this error message wasn't triggered because of the console but because of these 2 lines (or maybe both but making the command lazy didnt solve the issue):
https://github.com/algolia/search-bundle/blob/master/src/Resources/config/services.xml#L19-L20

[cryptiklemur]

@weaverryan any input?

[ogizanagi]

Actually I think the algolia_client service itself should be lazy, not the doctrine listeners, as just using the entity manager somewhere will trigger the listeners and client instantiation. symfony/symfony#27675 might solve this, but only using symfony 4.2+.
By making the client lazy, any service in which the client is injected will instead get a proxy instantiating the client only when really needed (as soon as one of its method is used). Probably worth it.

Also, in order for the lazy attribute to work, it requires the end-user to install the symfony/proxy-manager-bridge package

[julienbourdeau]

@ogizanagi I'd love to make the client lazy, but I'm starting to think that the easiest way would just be to remove the credentials from the service.xml, maybe even define the algolia_client service in the AlgoliaSearchExtension class :/

I also need to keep compatibility with Symfony 3.4.

[julienbourdeau]

I made this PR as a POC: #249

Any thoughts are welcome!

[weaverryan]

In Symfony 3.4 you could pass the listener a service locator that has the client inside. That would allow the event listener to be instantiated without instantiating the client. That’s how you can make a service lazily instantiated without using the lazy key on the service, which, yea, requires another lib :)

[ogizanagi]

Sure. It feels weird from a design POV (as there is always only one client currently), so that's why I suggested having a proxy instead, but that's the only viable solution to me not requiring the user to install another dependency.
That also means users should think were is injected the client, as it can still cause troubles on userland (in a CLI command for instance, remember #218 (comment) for instance if a command has the client injected) and should use a service locator as well while having a proxy would be no-brain (once the dep is installed of course). But this is not specific to this bundle.

[julienbourdeau]

So I explored the Service Locator option, it probably could work but in the end, the proxy solution when much much easier and clean. So even if I want to keep the number of depencies as small as possible, I believe this was worth it.

Thank you very much @ogizanagi @aequasi @weaverryan @OskarStark for the guidance and feedback, I really appreciate 🙏

Feel free to review and test #251