Add support for Guzzle 7 #24
Conversation
|
Hey are there any plans to merge this commit into the main repo soon? We've purchased this subscription and tried to setup locally to have this fail on the guzzle version, luckily its a simple change so I've forked this commit and tested this locally and it works great. Can we have this merged into the main repo soon rather than having to use a forked version? |
|
Same as @gutig here. |
|
Same as @gutig |
|
Sorry for the delay here folks. Thank you @airdrummingfool for the PR and workaround for the moment. We use https://github.com/swagger-api/swagger-codegen to generate our clients so we have more time for other improvements. I was really hoping they'd update this upstream by now, but I don't see activity. It looks like there's a fork of that project with more relevant activity:
I've 👍'ed those. Additional help pushing those would be greatly appreciated. If they don't see see activity soon we can look into fixing the issue with the linked PR, or switching to another approach entirely. |
Temp fix before upstream is fixed: DocRaptor#23 DocRaptor#24
Temp fix before upstream is fixed: DocRaptor#23 DocRaptor#24
|
It's been several months and still no fix. Is there any update? |
We've been following the upstream issues you cited, but haven't seen any movement there for the mast several months. Do you have plans to accept the linked PR or switch to another approach, or are you still hoping that the upstream project will resolve this for DocRaptor? |
|
I've definitely been hoping that upstream would resolve this, but that hope has dwindled dramatically over time. I've bumped up the internal priority of handling this, though we still have some other projects in-flight that we need to wrap up first. Ideally I think we push the upstream PRs over the line if they aren't merged yet, but we'll have to see what the difficultly of that looks like once we get into the details. If that looks too painful we'll probably merge this. |
|
@jason-o-matic It looks like the upstream issue you linked to has now been fixed. Can you please regenerate and release a fixed version of docraptor-php? |
|
That's great news! I've bumped this to the top of our queue internally and will update with more info as available. Thanks for the message! |
|
I've regenerated the client using OpenAPI 5.3.0 which supports Guzzle 7. As @christianl-das noted in #25, there are a few issues with the code as generated (e.g. blank pdfs). I think I've got all the issues worked out and just need to update the tests before releasing. Expect a new release after American Thanksgiving (November 25). |
Why is this change needed? -------------------------- PHP users want to support for Guzzle 7[1] How does it address the issue? ------------------------------ OpenApi added support for Guzzle 7 as of OpenApi 5.3.0. At a high level, this commit - Changes from Swagger to OpenApi - Regenerates the client using OpenApi 5.3.0 - Adds stronger verification in the tests that generated documents are valid and correct **BIG CAVEAT**: This client has been hand-edited in one important way. The DocApi functions `getAsyncDoc` and `createDoc` have always returned the file contents when called. Under OpenApi 5.3.0, the generated client instead returns an SplFileObject[2]. If you attempt to write an SplFileObject as if it werre the file contents, you get something that looks and acts like a blank PDF. That's a very confusing experience that could easily lead users to believe there are problems with the service or the agent. There are lots of good reasons to return an SplFileObject instead of the file contents, but in this case it doesn't seem worth it. In the best case, it means everyone that upgrades will need to make at least some changes to their integration. At worst, it creates a very confusing and frustrating failure. If you look at those two functions, you'll see where I've commented out the generated return and replaced it with the code necessary to maintain the current behavior. Since I made that change by hand, it will be destroyed when regenerated. If you're reading this because you've regenerated the client and the tests are failing, compare the code in the aforementioned DocApi functions from this commit to your newly generated code and make the appropriate changes. In this case, it is one line of fairly simple code. It should not be onerous to copy or integrate. Or, I guess you could decide SplFileObject is the way to go. The power is yours! In other news, here's a high level of groups of change. > .generator-revision, .openapi-generator-ignore, .openapi-generator These files are renamed because we switched generators. > lib/* Files generated by OpenApi > script/* Mechanical changes to support OpenApi instead of Swagger. Things like flag changes and referencing slightly different files that do basically the same thing they've always done. > test/* Each test that makes a document now also verifies that the document is a believable size. The real PDFs should be close to 30k. The "blank" pdf problem I mentioned above generated documents less than 1k. If we need a more robust test than this, we'll likely need to actually start decoding the PDF. Each test also writes the generated document to the local tmp directory so it's easier to inspect when you think something hinky is going on. I also refactored the tests a bit to make it easier to share and compare code. Hardcoded uses of the test name are replaced with a variable and the code for writing the generated file to the file system is now the same across each test. Any links to any relevant tickets, articles or other resources? --------------------------------------------------------------- 1 - #24 2 - https://www.php.net/manual/en/class.splfileobject.php
|
I released version 3.0 today. Thanks to @airdrummingfool for providing an interim fix, @christianl-das for trying to regenerate the client as soon as upstream released a fix, everybody that helped draw attention to this upstream, and to everybody else for hanging in there. It took a lot longer than we hoped for upstream to provide the needed support, so we're considering what changes we need to make to avoid delays like this in the future. In the mean time, please enjoy the new release! |
Guzzle 7 is mostly backwards-compatible with Guzzle 6 (see full upgrade guide). This change Works On My Machine (TM).
Fixes #23.