search

Prezent / doctrine-translatable

Translatable behaviour extension for Doctrine2
MIT License
26 stars 17 forks source link

XML driver #17

Closed gonzalovilaseca closed 9 years ago

gonzalovilaseca commented 9 years ago

This is a driver for xml mapping, if it's going to be merged I will add tests and doc.

gonzalovilaseca commented 9 years ago

PR updated, now using PHP DOMDocument

sandermarechal commented 9 years ago

It's looking good! I have added a few comments in the code. I would also very much appreciate a test case for this.

gonzalovilaseca commented 9 years ago

@sandermarechal Sorry, I forgot to update the PR with the latest changes, most of you comments where already done....I will create the test and update the PR.

gonzalovilaseca commented 9 years ago

PR has been updated with documentation and suggested changes.

As for the tests when I started with them I realised that the FileDriver class needs a refactoring, it's virtually impossible to mock collaborators so that only the XML metadata loading is tested in isolation.

I currently don't have the time for this right now, but will do it in the near future.

I'm not quite familiar with PhpUnit as I use PhpSpec, should I stick with PhpUnit or can I add the tests in PhpSpec?

As soon as this is merged I will send a PR to the Prezent Symfony Bundle.

sandermarechal commented 9 years ago

I will refactor the FileDriver for you and provide tests for the AnnotationDriver and YamlDriver. When I finished that it should be easy for you to provide a (PHPUnit) test for the XmlDriver.

gonzalovilaseca commented 9 years ago

That would be great, thanks.

gonzalovilaseca commented 9 years ago

@sandermarechal Do you have an estimate of when will you have the refactoring done? No pressure, just to know, as I need this PR merged so that the translations work in Sylius. If you're busy maybe you can merge this PR and add the tests in a following one.

sandermarechal commented 9 years ago

I will probably have it done today. The refactor itself is already done, but some code I split off from the FileDriver still needs to be updated.

gonzalovilaseca commented 9 years ago

Ok, let me know and I will also update the bundle PR.

sandermarechal commented 9 years ago

The refactoring has been complete. Can you rebase your changes on master? Providing a test case should be easy now.

Note that the Yaml mapping format has slightly changed. For a translation class, the 'locale' property is no longer a child of a field, but it is mapped similarly to the currentLocale and fallbackLocale. You may want to make the same change to your XML mapping. This makes it possible to separate the translation mapping from the ORM/ODM mapping if a user wishes.

sandermarechal commented 9 years ago

Hi. How is the rebase going? Don't worry about the bundle too much. I can fix that with a compiler pass.

gonzalovilaseca commented 9 years ago

done!

sandermarechal commented 9 years ago

Thanks. Just a few tiny points and then this can get merged.

gonzalovilaseca commented 9 years ago

@sandermarechal PR updated! Please merge it as soon as possible: there is a huge PR on Sylius that depends on this one.

pjedrzejewski commented 9 years ago

@sandermarechal Awesome, thank you for that! :+1:

sandermarechal commented 9 years ago

Thank you for your contribution and patience!