Sylius: PHP-PM issue with taxons setParent

Created on 3 Aug 2018  Â·  1Comment  Â·  Source: Sylius/Sylius

Sylius version affected: Probably affects all versions

Description
When running Sylius in PHP-PM I noticed that when creating child taxons, Sylius will crash because it tries to insert a taxon with all fields null.

This happens because the children collection of the parent taxon has 2 elements it tries to flush:

  • an empty taxon with all fields null (this causes the error)
  • the real taxon we are trying to add

The reason this happens is that the setParent method in Taxon changes the parent's state by adding to the children collection regardless if the form was submitted or not (e.g. simply viewing the form adds a child to the collection).

In a normal PHP request/response lifecycle this is not a problem because the parent would be discarded at the end of the request. However when using PHP-PM the parent is kept, so the empty taxons are never cleared and crash when the form is actually submitted.

Steps to reproduce
Run Sylius in PHP-PM and try to add a new taxon under a parent taxon.

Possible Solution
The problem here is that the parent is modified as a side of effect of calling createForParent - its not the actual resource we are creating. There are several approaches I can think of for solving this:

  • Remove createForParent completely and add the parent field to the form (though we need still to find a way to set its default value).
  • Change setParent to not call addChild inside the entity. We would call then addChild separately, from an event listener, if/when the form is valid and the child is submitted.
  • Add a reset method in Factory or ResourceController that would be triggered using Symfony's kernel.reset tag and would call $entityManager->clear or refresh on the objects. In this case we would need to somehow keep a reference of which objects to reset (e.g. the parent).

What do you guys think is the best solution? I would be happy to make a PR for this if we can decide the correct architecture.

PS. TaxonSlugController also calls the setParent method and has the same leak. Fixing it there is far simpler though.

Potential Bug

Most helpful comment

This and other similar issues should be now fixed by https://github.com/php-pm/php-pm-httpkernel/pull/127

PS. I have been running Sylius with PPM in production for the past week and there have been no issues so far after this fix. It really shows how well-crafted the Sylius codebase is to be able to run with only minor tweaks in a completely different execution model. Well done! 🎉

>All comments

This and other similar issues should be now fixed by https://github.com/php-pm/php-pm-httpkernel/pull/127

PS. I have been running Sylius with PPM in production for the past week and there have been no issues so far after this fix. It really shows how well-crafted the Sylius codebase is to be able to run with only minor tweaks in a completely different execution model. Well done! 🎉

Was this page helpful?
0 / 5 - 0 ratings

Related issues

igormukhingmailcom picture igormukhingmailcom  Â·  3Comments

eb22fbb4 picture eb22fbb4  Â·  3Comments

tchapi picture tchapi  Â·  3Comments

stefandoorn picture stefandoorn  Â·  3Comments

mikemix picture mikemix  Â·  3Comments