[Resource] Forms create resource objects without factories about syliusresourcebundle HOT 16 OPEN

@lchrusciel good thing you got stalebot - I almost forgot about this one. I'll see if I can cook up a BC compatible fix for 1.x and properly fix it for 2.0.

from syliusresourcebundle.

But it works just fine in the admin UI, you got some issues in the API?

from syliusresourcebundle.

That was quick, thanks!

The issue appears in the product options API, see the following test case: https://github.com/Sylius/Sylius/pull/7547/files#diff-8a036ee49b21796b22c16f8e5017ac28R100

When adding getValue as virtual property in the ProductOptionValue serializer configuration (see the above PR for an example), this causes an error because the ProductOptionValue object was not created through a TranslatableFactory but rather as new ProductOptionValue(), which doesn't set the required locale fields.

Since it's recommended to create objects through their respective factories, I'd say the forms should do the same thing instead of relying on constructors.

from syliusresourcebundle.

@alcaeus As I told you before, we should not use virtual properties on translatable properties, just expose all translations. Current locale and fallback locales are not stored in the database, so it is not a problem to create ProductOptionValues and persist them without these values. The problem that you have encountered appears because getName and getValue requires these values if no argument has been given, but because ProductOptionValues has been just created and is not reloaded from the database. If you would add this object to the database and then show it in the next request everything would work. ORMTranslatableListener will set both values during load.

Probably we could consider such a change before, but not at this stage. Especially, when we can live without it.

I have also opened a PR to your branch with the fix to your example.

from syliusresourcebundle.

Thanks for your reply, Łukasz. "Persist and reload the object from the database" is not a solution, it's a workaround, especially if it's not something I can't control. In this case, the form creates an object and returns it, I don't expect to have to reload it from the database in order to be able to fully use it. After all, the Sylius docs on creating resources explicitly states that "to create new resources you should use default factory implementation", followed by a lengthy explanation why using the respective factories is better than calling new MyResource(). I'd expect the forms to follow this convention.

from syliusresourcebundle.

@alcaeus, of course, you are right. The problem didn't raise earlier because for most of the cases we are providing a properly created object to form factory(which is more convenient) and for the rest of objects, that are translatable and created within the collection(which is rare) we actually do the persist and reload(save and redirect). What I am saying is that it is pretty late for this change.

from syliusresourcebundle.

IMHO we can replace $dataClass from the constructor definition with FactoryInterface $factory. data_class option can be computed by get_class($factory->createNew()) and then empty_data option will be a closure using that factory.

But is it a quite big BC break though.

from syliusresourcebundle.

Yes, the BC break is the issue. Of course, this could be avoided by making the factory optional (while still specifying them for every default resource), but that could still cause issues for users.

Another downside of not using factories is that the forms (e.g. the ProductOptionValueType) will disregard any changes to the resource class. Overriding the ProductOptionValue class in the resource configuration would have no effect since the form type still creates the default object.

from syliusresourcebundle.

@pamil @pjedrzejewski @lchrusciel any suggestions on how to proceed? It would be good if we could solve this without a BC break. I can work on it, I just need an indication on the solution you'd prefer.

from syliusresourcebundle.

Any update on this? With 1.0 out the door BC breaks are out of the question, but I still think the issue should be fixed. Any indications what direction you want to go?

from syliusresourcebundle.

@alcaeus sorry, but we didn't manage to change it before 1.0 Probably we can add it to 2.0 scope. I'm not sure when some of us will have time for that

from syliusresourcebundle.

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

from syliusresourcebundle.

Thanks a lot Andreas!

from syliusresourcebundle.

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

from syliusresourcebundle.

🔥

from syliusresourcebundle.

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

from syliusresourcebundle.