search

maschmann / TranslationLoaderBundle

Symfony2 bundle with database translation loader
23 stars 9 forks source link

Circular reference #20

Closed xomi closed 9 years ago

xomi commented 10 years ago

Hello, I'm using your bundle in a Symfony 2.4 project, and everything was fine until I started accessing the db, in this case authenticating user, when I get this error:

ServiceCircularReferenceException: Circular reference detected for service "doctrine.orm.default_entity_manager", path: "doctrine.orm.default_entity_manager -> doctrine.dbal.default_connection -> asm_translation_loader.history.subscriber -> security.context -> security.authentication.manager -> security.user.provider.concrete.usuarios".

I'm just trying to login users with an entity provider. If I use a memory provider, it all works fine.

Thanks in advance,

Toni.

maschmann commented 10 years ago

Hi @xomi, will check that...

xomi commented 10 years ago

Hi @maschmann, I have been investigating and found this: http://stackoverflow.com/questions/8708822/circular-reference-when-injecting-security-context-into-entity-listener-class I think this should be the solution, changing the injection of the security context to the full container object.

maschmann commented 10 years ago

@xomi , thanks for investigating :-) But injecting the container is considered bad practice since it creates enormous overhead and also decreases testability due to relying on the whole container and therefore increasing the dependencies of the package enormously. I'm looking for another solution to maybe circumvent the injection at that stage (listener) completely.

// edit I've just stumbled upon last nights post from Matthias Noback, which might solve this case: http://php-and-symfony.matthiasnoback.nl/

xomi commented 10 years ago

Nice coincidencw un time :-)

xomi commented 10 years ago

Nice coincidente in time, I wanted to say (Damn spanish corrector and predictive text for mobile phones...)

xabbuh commented 10 years ago

@xomi I tried to reproduce your issue but wasn't able to do so. Can you show your configuration and describe any additional steps necessary to reproduce the issue?

EDIT: Never mind, I can reproduce it. Working on a fix.

xomi commented 10 years ago

@xabbuh Sorry for the delay, I wasn't at my computer this afternoon. Glad to hear you could reproduce it. It's a very simple scenary, just trying to protect some pages with a login base on an entity and error happens. If you need any conf file or anything else, just ask for it.

Thanks.

xabbuh commented 10 years ago

@xomi Starting with 63bff649226f74706fc665b9b9cb4252dc161424 we don't load the history subscriber service unless it is explicitly enabled (it is enabled by default). You can use 1.0@dev as a version constraint in your composer.json file until we have a final fix.

xomi commented 10 years ago

@xabbuh Works ok as temporal patch. Waiting for final fix. I love your work, guys.

Thank you very much.

xabbuh commented 9 years ago

@xomi Can you check if the bundle is working with the fix proposed in #22 even when the history subscriber is enabled?

xomi commented 9 years ago

@xabbuh Sorry for my ignorance, but... How can I get the branch in my composer to test it?

xabbuh commented 9 years ago

@xomi You had to clone my fork of the repository. Sorry, that was not very user-friendly. I also pushed the branch here. So, you can now add it as a version in your composer.json file and run composer update:

{
    "require": {
        "asm/translation-loader-bundle": "dev-issue-20"
    }
}
xomi commented 9 years ago

@xabbuh Ok, installed and testing. But it doesn't seem to work on my installation. When I try to use this last version, it returns an error, but this does not fire the error dispatcher, but returns the error page into the variable that made the error. I know I didn't explain it very well, so better 'copy&paste' the result and you'll see the error and the way it returns it.

Note that I'm trying to translate a variable called 'title' that contains the title of the webpage.

<!DOCTYPE html> <!-- HTML5 Boilerplate --> <!--[if lt IE 7]><html class="no-js lt-ie9 lt-ie8 lt-ie7" lang="es"> <![endif]--> <!--[if (IE 7)&!(IEMobile)]><html class="no-js lt-ie9 lt-ie8" lang="es"><![endif]--> <!--[if (IE 8)&!(IEMobile)]><html class="no-js lt-ie9" lang="es"><![endif]--> <!--[if gt IE 8]><!--> <html class="no-js" lang="es"><!--<![endif]--> <head> <meta charset="UTF-8" /> <title><!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/> [here goes more heading, styling, ...] <h1>Whoops, looks like something went wrong.</h1> <div class="block_exception clear_fix"> <h2>``<span>1/1</span> <abbr title="Symfony\Component\Debug\Exception\ContextErrorException">ContextErrorException</abbr>: Catchable Fatal Error: Argument 3 passed to Asm\TranslationLoaderBundle\Doctrine\TranslationManager::__construct() must implement interface Symfony\Component\EventDispatcher\EventDispatcherInterface, none given, called in /home/www/webs/carnivale/app/cache/dev/appDevDebugProjectContainer.php on line 364 and defined in /home/www/webs/carnivale/vendor/asm/translation-loader-bundle/Asm/TranslationLoaderBundle/Doctrine/TranslationManager.php line 44</h2>

xabbuh commented 9 years ago

@xomi Sorry, I didn't push all modified files. Can you please update and try again?

xomi commented 9 years ago

@xabbuh Ok. I'll try it tomorrow. Thanks

xomi commented 9 years ago

@xabbuh I was trying it, and it seem to work fine . Circular reference error has not appeared, and it updates successfully translations. However, I don't know if it's a new or older issue because I had not tried it before, but when setting history to true in config.yml it registers nothing in translation_history table.

Thank you very much

xabbuh commented 9 years ago

@xomi Than you very much for your help. I really appreciate that. I'll take a look at the values not being stored.

xabbuh commented 9 years ago

@xomi Indeed, this should be fixed now. I'll be happy if you could try it again.

xomi commented 9 years ago

@xabbuh Haven't you commited it into the 'issue-20' branch?

xabbuh commented 9 years ago

@xomi Yes, I did a forced push. But composer update should update it.

xomi commented 9 years ago

But it didn't...

xabbuh commented 9 years ago

Sorry, didn't update this remote. Can you try it again?

xomi commented 9 years ago

@xabbuh I'm trying it right now...

xomi commented 9 years ago

@xabbuh It seems not to be fine yet. When updating a value, it records the change at 'translation-history' table succesfully, but it throws an exception updating 'translation' table:

An exception occurred while executing 'INSERT INTO translation (translation, date_created, date_updated, trans_key, trans_locale, message_domain) VALUES (?, ?, ?, ?, ?, ?)' with params ["info@c.com", "2014-08-01 12:00:00", "2014-09-12 21:05:05", "direccion.email", "en", "messages"]:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'direccion.email-en-messages' for key 'PRIMARY'

xabbuh commented 9 years ago

Can you show some code? It seems like your are trying to insert the same translation twice.

xomi commented 9 years ago

@xabbuh Of course. It's the same code that worked till now. I have an entity called Lineatraduccion that handles two translations at once, just to use it at a form and set all translations in the same key and domain at the same time. The most relevant code is pretty simple:

/**
 * Set transKey
 *
 * @param string $transKey
 * @return Lineatraduccion
 */
public function setTransKey($transKey)
{
    $this->transKey = $transKey;

    return $this;
}
/**
 * Set datosComunes
 *
 * @param Traduccion $traduccion
 * @return Lineatraduccion
 */
public function setDatosComunes(Translation $traduccion)
{
    $this->setTransKey($traduccion->getTransKey());
    $this->setMessageDomain($traduccion->getMessageDomain());

    return $this;
}

/**
 * Set lineaEntera
 *
 * @param Traduccion $traduccionEs, $traduccionEn
 * @return Lineatraduccion
 */
public function setLineaEntera(Translation $traduccionEs, Translation $traduccionEn)
{
    $this->setDatosComunes($traduccionEs);
    $this->setTranslationEs($traduccionEs->getTranslation());
    $this->setTranslationEn($traduccionEn->getTranslation());

    return $this;
}

And, in the controller...

public function traduccionAction(Request $peticion, $dominio, $clave)
{
    $repository = $this->get('asm_translation_loader.translation_manager');
    $textoEs = $repository->findTranslationBy(['messageDomain' => $dominio, 'transKey' => $clave, 'transLocale' => 'es']);
    $textoEn = $repository->findTranslationBy(['messageDomain' => $dominio, 'transKey' => $clave, 'transLocale' => 'en']);
    $linea = new Lineatraduccion;
    $linea->setLineaEntera($textoEs, $textoEn);
    $formulario = $this->createForm(new TranslationType, $linea);

    $formulario->handleRequest($peticion);
    if ($formulario->isValid()) {
        $textoEs->setTranslation($linea->getTranslationEs());
        $textoEn->setTranslation($linea->getTranslationEn());

        $repository->updateTranslation($textoEs);
        $repository->updateTranslation($textoEn);

        $this->get('session')->getFlashBag()->add('success', 'La traducción se ha guardado correctamente.');
        $this->get('session')->getFlashBag()->add('cache', 'Se han hecho cambios en la traducción. Para que estos se reflejen en la página, es necesario <a href="' . $this->generateUrl('be_cache', array()) . '">limpiar la caché</a>.');
        return $this->redirect($this->generateUrl('be_textos', ['dominio' => $dominio]));
    }
    return $this->render('BackendBundle:Default:traduccion.html.twig', array(
        'mensajes'      => $this->sacaMensajes(),
        'formulario'    => $formulario->createView(),
        'dominio'       => $dominio,
        'clave'         => $clave,
    ));
}
xomi commented 9 years ago

@xabbuh The exception jumps when executing the second 'updateTranslation'. If I comment the second line:

// $repository->updateTranslation($textoEn);

it works strangely. Because it updates the two records fine, but only registers one translation-history record, obviously the not commented one...

As you can see, I don't insert any records, just updating them... The amazing thing is that it updates the two translations succesfully...

xabbuh commented 9 years ago

Yeah, you were right. The doctrine manager was accidentally cleared in the history manager. This should be fixed now.

xomi commented 9 years ago

@xabbuh That's solved. But that strange behaviour of updating every translation still happens. Now there is no exception, but if I comment one of the two updateTranslation, that translation does not appear at translation-history, but it's updated in translation. I don't worry too much about it, because I'll keep the two lines, but it seems strange, don't you think?

Guy, I really owe you a beer (or two...). Thank you very much.

xabbuh commented 9 years ago

@xomi Not really, you can observe the same behaviour when using Doctrine only. Since you fetch them (indirectly) via the repository they are managed by Doctrine. Since flush() is called inside updateTranslation() all managed objects that have been modified in the meantime will be written to the database.

Of course, we could modify the updateTranslation() method to optionally disable the call to flush() (basically this would be similar to what the FOSUserBundle does in the UserManager). What do you think about it?

xomi commented 9 years ago

@xabbuh I understand, and agree about the call to flush(), but not about calling a persist($entity).

I mean, following my case as example, I'd expect that calling $repository->updateTranslation($textoEs), it only will persist the object $textoEs and then flush() every pending to flush object.

Not sure if I'm right or not, but that's what I expect when I pass the object as a parameter to the update function.

After all, there could be a method to persist and flush all changes, but this method wouldn't require the object instance as parameter.

Don't you see it the same way?

xabbuh commented 9 years ago

I merged the pull request and published the 1.0.4 release. So, you no longer have to rely on a development version, but you can switch back to a version constraint like ~1.0.

If you think that the behaviour of the updateTranslation() method needs some more discussion, please feel free to open a new issue for it.

Thanks for your report, @xomi!