8000 Lazy-load commands by ogizanagi · Pull Request #218 · algolia/search-bundle · GitHub
[go: up one dir, main page]

Skip to content

Lazy-load commands #218

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

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Conversation

ogizanagi
Copy link
Contributor

so the algolia client isn't instantiated uselessly or too soon.

Some refs:


The algolia_client needs the ALGOLIA_APP_ID & ALGOLIA_API_KEY env vars. But when building & releasing my application, this vars are not necessarily set (they are only set and available on the server, hence once the release is deployed). Each of these commands need the index or settings managers, which in turn require the client as dependency. Which means you'll get a:

Environment variable not found: "ALGOLIA_APP_ID".

by running any command registered in the application.
By lazy-loading the algolia commands, none of their dependencies are instantiated unless really required.

@julienbourdeau
Copy link
Contributor

Great contribution! I'll have a look shortly but I think this will also fix #199

Thanks a lot for contributing.

@julienbourdeau julienbourdeau merged commit 627d26e into algolia:master Mar 22, 2018
@julienbourdeau
Copy link
Contributor

Thanks!

The doc states that the list comment still need to instanciate each command so you'll still have undefuned env variables but I think it's a great addition 🎉

@ogizanagi ogizanagi deleted the lazy_load_commands branch March 22, 2018 11:34
@OskarStark
Copy link

The doc states that the list comment still need to instanciate each command so you'll still have undefuned env variables but I think it's a great addition 🎉

any chance to change this?
My problem is, that I don't have access to my ENV vars at build time, but using this in my services.yaml:

services:
    _defaults:
        public: true

    AlgoliaSearch\Client:
        arguments: ['%env(ALGOLIA_APP_ID)%', '%env(ALGOLIA_API_KEY)%']

the only way to use it like this is to define it at build time to, which requires a commit of the APP_ID and API_KEY to the repo... 😢

@ogizan
8000
agi
Copy link
Contributor Author

any chance to change this?

Unfortunately, that's unlikely to happen as the list command requires to access more than the command name (namespace, description, ...).

Would be better to find a solution to lazy load the algolia client or another service up in the stack to prevent its instantiation when not strictly required.
One quick solution could be to set the lazy flag on the client service definition, so people installing the ocramius proxy-manager would get a proxy injected in their service, until really used. Another way is using service locators. But I don't see where it would fit.

@OskarStark
Copy link
OskarStark commented Mar 23, 2018

@ogizanagi thats exactly what I tried, but it is not working:

ocramius/package-versions            1.3.0   Composer plugin that provides efficient querying for installed package versions (no runtime IO)
ocramius/proxy-manager               2.2.0   A library providing utilities to generate, instantiate and generally operate with Object Proxies
symfony/symfony                      v3.4.6  The Symfony PHP framework
algolia/algoliasearch-client-php     1.25.1  Algolia Search API Client for PHP
algolia/search-bundle                3.1.1
    AlgoliaSearch\Client:
        arguments: ['%env(ALGOLIA_APP_ID)%', '%env(ALGOLIA_API_KEY)%']
        lazy: true

error:
screenshot 2018-03-23 09 35 24

do I need to activate the lazy stuff?

@ogizanagi
Copy link
Contributor Author
ogizanagi commented Mar 23, 2018

No need to enable anything as soon as the ocramius proxy manager & bridge is installed. Check if any service is doing something with the client in the constructor or if a service is likely to use it during cache warmup.

@OskarStark
Copy link
OskarStark commented Mar 23, 2018

Damn, i guess this is breaking it, and I don't need to define the service on my own!
https://github.com/algolia/search-bundle/blob/master/src/Resources/config/services.xml#L18-L21

WDYT of making the client lazy by default in this bundle?

We could add ocramius/proxy-manager as hard dependency or add a CompilerPass to this bundle and set it lazy if ocramius/proxy-manager is used in the project

This works for me:

<?php

namespace AppBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

class AlgoliaCompilerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        if ($container->has('algolia_client')) {
            $definition = $container->getDefinition('algolia_client');
            $definition->setLazy(true);
        }
    }
}

@ogizanagi
Copy link
Contributor Author

Setting the lazy flag on the algolia client service definition and let the user install ocramius proxy-manager if he needs it looks a good workaround for now.

@OskarStark
Copy link

Setting the lazy flag on the algolia client service definition and let the user install ocramius proxy-manager if he needs it looks a good workaround for now.

yeah, you are right, we can set it directly on the service definition, no need for a compiler pass!

I will provide a PR if you are fine with it @julienbourdeau

@julienbourdeau julienbourdeau mentioned this pull request Mar 28, 2018
< 91B7 span title="Status: Closed" data-view-component="true" class="State State--merged State--small d-flex flex-items-center"> Closed
@julienbourdeau
Copy link
Contributor

@OskarStark @ogizanagi Thank you for looking into this! It would be best to move the conversation to #241 Sorry I didn't get back to you about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0