Use Rust FFI #14: Xml example - #335
Conversation
| "path": "/movies" | ||
| }, | ||
| "response": { | ||
| "body": "<?xml version='1.0'?><movies><movie><title>PHP: Behind the Parser</title><characters><character><name>Ms. Coder</name><actor>Onlivia Actora</actor></character><character><name/><actor/></character></characters><plot>So, this language. It's like, a programming language. Or is it a\nscripting language? All is revealed in this thrilling horror spoof\nof a documentary.</plot><great-lines><line>PHP solves all my web problems</line></great-lines><rating type='thumbs'>7</rating><rating type='stars'>5</rating></movie></movies>", |
| namespace XmlConsumer\Tests\Service; | ||
|
|
||
| use Tienvx\PactPhpXml\Model\Options; | ||
| use Tienvx\PactPhpXml\XmlBuilder; |
There was a problem hiding this comment.
We will probably want to document that this comes from a seperate-repo that would need to be added, into a readme of the changes.
https://github.com/tienvx/pact-php-xml
ps, what is the advantage of publishing these are separate packages?
Is it useful for
- users of the library?
- testing of the library?
or are there other advantages, outside of the possibly maintainence sprawl of having lots of different projects.
It might be that it works for PHP and is what users expect, but equally leads to more places to maintain, and more things to convey to the user, that they need, conditionally, depending on their particular use case
There was a problem hiding this comment.
The reason I put some code outside of this PR, into separate project pact-php-xml because:
- I just want to make this PR small so it easier to review
- The code of that library is highly opinionated
- That library is optional, can easily replace by raw json
What do you think if I move the code back to this PR?
There was a problem hiding this comment.
Not sure on a definitive answer, but good question and counter points. I've tagged a few other parties who may be offer to offer their thoughts.
On the fact its optional and highly opinionated, would you see this kind of library either coming under the pact-foundation, or us having a list of them in the repo, so that others can create their own and point to them (many ways to skin a cat and all that!)
Hey team, just wondering if you can cast your eyes on the above thread. 🙏🏾
There was a problem hiding this comment.
reading now, but feel it would be better for Pact to have this repo under their umbrella with a contributor to review (there might be a lot I'd miss as I dip in and out)
There was a problem hiding this comment.
Regarding the specifics, it's a value-object using builder; it's a strange mix. Why this vs templating? Unless I'm mistaken (in which case I apologise); but it looks like a special string builder. XML is not strictly speaking a string.
There was a problem hiding this comment.
I believe it to be a special string builder, as we need to store the XML in the pact json file, so its stored in a string (along with any matchers that have been encoded as part of the test)
This is unpacked at the provider side, and reads the content type of application/xml and sends the content appropriately to the provider
There was a problem hiding this comment.
json_encode($iContainXML) should handle escaping etc though right?
I guess we need to know if pact needs UTF-8 escaped, but XML does not and JSON spec does not, so I'm guessing not. 😄
There was a problem hiding this comment.
I updated the builder to use the Functional Options pattern.
There was a problem hiding this comment.
- About UTF-8 encoding:
- According to this safety note, I think yes, pact (in this case
pactffi_with_body) need UTF-8 encoding. - According to this doc I think
json_encodealso need the value to be UTF-8 encoded.
- According to this safety note, I think yes, pact (in this case
- About UTF-8 encoding vs escaping: I'm not good at this. I don't think I handle these things at all in the code. I think we need to add special test cases and then update the code to handle these cases.
YOU54F
left a comment
YOU54F
left a comment
There was a problem hiding this comment.
Looks good, comment on some docs re pulling in pact-php-xml project but bar that sweet
|
Is there a concrete example of the XML this produces, or the "string with XML" this produces? I'd very much like to compare it to |
Pact's Rust core (FFI) required the body in JSON format (not XML). Here is the json string (json encoded from builder's array): {
"version": "1.0",
"charset": "UTF-8",
"root": {
"name": "movies",
"children": [
{
"pact:matcher:type": "type",
"value": {
"name": "movie",
"children": [
{
"name": "title",
"children": [
{
"content": "PHP: Behind the Parser",
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {}
},
{
"name": "characters",
"children": [
{
"pact:matcher:type": "type",
"value": {
"name": "character",
"children": [
{
"name": "name",
"children": [
{
"content": "Ms. Coder",
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {}
},
{
"name": "actor",
"children": [
{
"content": "Onlivia Actora",
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {}
}
],
"attributes": {}
},
"examples": 2
}
],
"attributes": {}
},
{
"name": "plot",
"children": [
{
"content": "So, this language. It's like, a programming language. Or is it a\\nscripting language? All is revealed in this thrilling horror spoof\\nof a documentary.",
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {}
},
{
"name": "great-lines",
"children": [
{
"pact:matcher:type": "type",
"value": {
"name": "line",
"children": [
{
"content": "PHP solves all my web problems",
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {}
},
"examples": 1
}
],
"attributes": {}
},
{
"name": "rating",
"children": [
{
"content": 7,
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {
"type": "thumbs"
}
},
{
"name": "rating",
"children": [
{
"content": 5,
"matcher": {
"pact:matcher:type": "type"
}
}
],
"attributes": {
"type": "stars"
}
}
],
"attributes": {}
},
"examples": 1
}
],
"attributes": {}
}
}
I'm not sure I understand this one. |
|
@tienvx the thing you are building the XML example for. I'd like to see what PHP's native |
Lewiscowles1986
left a comment
Lewiscowles1986
left a comment
There was a problem hiding this comment.
Now it doesn't need coordination of ->start() and ->end() calls I think this is a lot easier to work with. The extra package pinned to dev main will need to have something happen before it can be released as that is an attack vector; but I see no problem having this go into main
|
Please review and merge #340 to make tests pass |
|
@tienvx it's one line you made into another PR; the rest is just refactoring examples to be DRY (they don't have to be, they are separate examples). Does that one line have to be in another MR, or can it just be made here? |
|
I've approved the other MR, please rebase this from that branch once #340 is merged, providing this meets the PACT foundation approval as well. |
|
hmm, I rebased and merged this but this didn't close, maybe because I didn't push the rebase back to the this/original branch? Following gh instructions in the merge command section above, albeit with the additional rebase git checkout -b tienvx-xml ffi
git pull git@github.com:tienvx/pact-php.git xml
git rebase -i ffi
<rebase things>
git checkout ffi
git merge --no-ff tienvx-xml
git push origin ffi |
I think so. But if we push back to the branch of this PR, this PR will become empty. I think we just need to close this PR.
I'm happy to transfer, but I think this require more work to do, and I didn't do it before:
Please review that library, are there anything I need to update? then I will tag a version, probably |
agreed, we just hadn't agreed on a path forward atm but thanks for raising again I using a released tagged version that can be pinned to is sensible. It is sufficient to be from a git repo tag, rather than being published to packagist, especially whilst in our alpha phase?
Licenses across the foundation are a mix of either MIT, or Apache 2.0, and I think everyone is generally comfortable with that. related comment in maintainers guide draft pact-foundation/docs.pact.io#270 (comment)
Yes I believe you are correct, that you cannot rename. That is why I believe the original pact-php packages are under mattermacks handle. I am okay with this being outside of the pact foundation and transferred in when you are ready Tien. It's not a core piece piece of the project (not all users want/need the xml func) and it is added as a dev dep. Up to you! If it gets transferred in, you will have admin rights anyway, as its your own repository. We've not taken admin rights away from users who have transferred their repos in, as they are ultimately the original owners and we are but caretakers!
I would probably start with a <1.x version, until you get some real-world feedback, as users can take a v1.x library as production ready, but that is a matter of personal choice. Given the below thoughts, it is probably worth tracking this is a separate issue, so we can close this PR down and avoid ancillary noise
|
|
I release version 0.1.0 of Closed as commits are rebased to |
3 participants
Add xml example.
This PR depends on #332. Please review it first.