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.

Sign up for GitHub

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

Jump to bottom

Feature middlewares #23

Merged
merged 37 commits into from Feb 3, 2017
Merged

Feature middlewares #23

merged 37 commits into from Feb 3, 2017

Conversation

veewee
Copy link
Contributor

@veewee veewee commented Dec 27, 2016 •
edited

This PR adds middleware support to the soap-client so that it is easy to configure extensions.
More information can be found in #21.

Task list:

  • Specify a middleware layer
  • Apply handlers for triggering the SOAP requests
  • Adjusted the Soap-client to work with handlers
  • Added a Soap handler
  • Added a Guzzle handler
  • Changed the client builder to specify your own handler.
  • Changed the client builder to specify your own middlewares.
  • Created a http-binding based on https://github.com/meng-tian/soap-http-binding
  • Add NTLM middleware
  • Add Basic Auth middleware
  • Add WSA middleware
  • Add WSSE middleware
  • Added an easy way to load, read and change SOAP XML before sending or after receiving it.
  • Add spec + integration tests
  • Find a good way to test the middlewares
  • Make sure the debug() method returns correct values when using the GuzzleHandler
  • Add documentation
  • Normalize exception handling
  • Cleanup the codebase
  • End-2-End test with WSA + WSSE

Knows limitations:

@veewee veewee added this to the 0.3.0 milestone Dec 27, 2016
@veewee veewee self-assigned this Dec 27, 2016
janvernieuwe
janvernieuwe requested changes Jan 12, 2017
View reviewed changes
Copy link
Member

@janvernieuwe janvernieuwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add more scalar type hints where possible for consistency.

Align key value pairs over the whole project for consistency.

src/Phpro/SoapClient/Middleware/Middleware.php
*/
public function __invoke(callable $handler)
{
return (function (RequestInterface $request, array $options) use ($handler) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very hard to read, can this possibly be simplified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. This is basically the regular implementation of guzzle middlewares: http://docs.guzzlephp.org/en/latest/handlers-and-middleware.html#middleware
I made this class to make it easy to implement your own middleware, but the complexity has to be placed somewhere.

spec/Phpro/SoapClient/Soap/HttpBinding/LastRequestInfoSpec.php Outdated
{
$request = new Request('/', 'POST', 'php://temp', ['x-request-header' => 'value']);
$request->getBody()->write('REQUESTBODY');
$respone = new Response('php://memory', 200, ['x-response-header' => 'value']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo respone -> response

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

spec/Phpro/SoapClient/Soap/HttpBinding/LastRequestInfoSpec.php
function it_can_load_from_psr7_request_and_response_without_body()
{
$request = new Request('/', 'GET', 'php://temp', ['x-request-header' => 'value']);
$respone = new Response('php://memory', 204, ['x-response-header' => 'value']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/Phpro/SoapClient/ClientBuilder.php
$soapClient->setHandler($this->handler);
}

if (count($this->middlewares)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this condition be combined, and the foreach out of the if statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree on your argument. I've just changed it, but it was more readible as it is right now. I'll keep this one AS-IS for now.

src/Phpro/SoapClient/Middleware/WsseMiddleware.php Outdated
/**
* @var string
*/
private $privateKeyFile = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why assign an empty string?
This param is required in the constructor.

src/Phpro/SoapClient/Middleware/WsseMiddleware.php Outdated

$xml = SoapXml::fromStream($response->getBody());
$wsse = new WSSESoap($xml->getXmlDocument());
$wsse->decryptSoapDoc($xml->getXmlDocument(), [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align k/v pairs (formatting)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/Phpro/SoapClient/Soap/HttpBinding/Builder/Psr7RequestBuilder.php Outdated
foreach ($this->prepareHeaders() as $name => $value) {
$request = $request->withHeader($name, $value);
}
} catch (\InvalidArgumentException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use it, don't use fqcn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/Phpro/SoapClient/Soap/HttpBinding/Builder/Psr7RequestBuilder.php Outdated
* @var RequestFactoryInterface
*/
private $requestFactory;
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline for max style points

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/Phpro/SoapClient/Soap/HttpBinding/Builder/Psr7RequestBuilder.php Outdated
}

/**
* SOSPAction header is removed in SOAP 1.2 and now expressed as a value of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo?
Also the link doesn't work for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/Phpro/SoapClient/Xml/SoapXml.php
*/
class SoapXml
{
const XMLNS_XMLNS = 'http://www.w3.org/2000/xmlns/';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit weird, is this by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is by design: The XLMNS of alias XMLNS is the url.
This way, you can e.g. also add: XMLNS_WSA = 'url'; in a future version.

@veewee
Copy link
Contributor Author

veewee commented Jan 27, 2017

Thanks for the review @janvernieuwe. It feels like it's quite ready to ship! :)
About the strict type hints: currently there is still an older part in the code which was written for php 5.6. I'll add an issue to use strict types everywhere. That is out of the scope of this ticket.

@veewee veewee changed the title [WIP] Feature middlewares Feature middlewares Jan 27, 2017
This was referenced Jan 27, 2017
Closed
Closed
@veewee veewee merged commit 8229e08 into phpro:master Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janvernieuwe janvernieuwe janvernieuwe requested changes

Assignees

@veewee veewee

Labels
new feature
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@veewee @janvernieuwe