Skip to content
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

add options property to XmlDeserializationVisitorFactory and XmlDeserializationVisitor, propagate defined value from factory to simplexml_load_string call #1068

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

kopeckyales
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #828
License MIT

…ializationVisitor, propagate defined value from factory to simplexml_load_string call
@goetas
Copy link
Collaborator

goetas commented Apr 2, 2019

Hi!
This looks a pretty nice thing to have! Thanks!

Do you mind adding some tests for it?

@kopeckyales
Copy link
Contributor Author

Hi goetas, thank you for quick response.
For me, only option how to test this is something similar to this: php_mock_global_functions_for_unit_tests_with_phpunit
But it is like 50/50 because this approach seems strange for me.

I was thinking about "mock" the simplexml_load_string function and save given options parameter to the global value and then in test check if options parameter was propagated correctly.
This is IMHO bulletproof test, but as I said, I do not really like the approach.

If you are satisfied with mentioned solution, I can implement it.

@goetas
Copy link
Collaborator

goetas commented Apr 10, 2019

Instead of mocking the behavior of simplexml_load_string, you can try to pass some option to the visitor, then call 'prepare', and check if the return of it followed the $options rules

@kopeckyales
Copy link
Contributor Author

kopeckyales commented Apr 10, 2019

Of course, your solution is possible, but in my opinion it is not suitable test. What if simplexml_load_string changes in the future and it will return different result for whatever reason? It breaks the test even if nothing in your code was changed. And of course it does not guarantee all options are propagated correctly, just the one you are testing. For me, this is almost comparable to have no test and it is the reason why i searched for a something more bullet-proof. I only found the given solution.

@goetas goetas merged commit 952070d into schmittjoh:master Apr 10, 2019
@goetas
Copy link
Collaborator

goetas commented Apr 10, 2019

I think we can keep it as it is.
Nor happy with both solutions. :/ but would like to have this feature

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

Successfully merging this pull request may close these issues.

2 participants