-
Notifications
You must be signed in to change notification settings - Fork 3
NoPrivateNetworkHttpClient decorator #35
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
NoPrivateNetworkHttpClient decorator #35
Conversation
233e90d
to
b9159af
Compare
Can you please rebase on the latest version of master? There is some git history issue here apparently :) |
b9159af
to
e727470
Compare
Done. |
e727470
to
aa3f856
Compare
released v3.4.37
released v4.3.10
released v4.4.3
released v5.0.3
… a double variable...) when accessing the cache panel (@see symfony#35419)
Make sure the `$cache->release()` method exists before executing it.
This PR was merged into the 4.4 branch. Discussion ---------- [HttpKernel] Check if lock can be released | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix symfony#35421 | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> [HttpKernel] Make sure the `$cache->release()` method exists before executing it Commits ------- 2001e54 [HttpKernel] Check if lock can be released
* 4.4: Bump phpunit-bridge cache
* 5.0: Bump phpunit-bridge cache
…rverBundle (jrushlow) This PR was squashed before being merged into the 5.1-dev branch (closes symfony#35538). Discussion ---------- [FrameworkBundle] fixed suggesting deprecated WebServerBundle | Q | A | ------------- | --- | Branch? | 5.0 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix symfony#35495 | License | MIT | Doc PR | Removed suggestion to install `symfony/web-server-bundle` in console missing packages suggestions. The web server bundle was deprecated and no longer works with Symfony 5.x, . Commits ------- 134129b [FrameworkBundle] fixed suggesting deprecated WebServerBundle
released v4.4.4
released v5.0.4
…or interface and implementation on reflection (joelwurtz, Korbeil) This PR was merged into the 5.1-dev branch. Discussion ---------- [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#30248, partially: symfony#22190, symfony#18016, symfony#5013, symfony#9336, symfony#5219, | License | MIT | Doc PR | TODO This PR brings accessor / mutator extraction on the PropertyInfo component, There is no link to existing code, as IMO it should be in another PR as this will add a dependency on property access to the property info component and not sure this is something wanted (although, it will reduce a lot of code base on the property access component as a lot of code seems to be duplicated) Code is extracted from symfony#30248 also there is some new features (that can be removed if not wanted) * Allow extracting private accessor / mutator (will do a new PR that improve private extraction on reflection latter) * Allow extracting static accessor / mutators * Allow extracting constructor mutators Current implementation try to be as close as the PropertyAccess implementation and i did not reuse some methods already available in the class as there is some differences in implementation, but maybe it will be a good time to make this consistent (Looking forward to your input) ? Things that should be done in a new PR: * Linking property info to property access to remove a lot of duplicate code * Add a new system that allow adding Virtual Property based on this extractor Commits ------- 0a92dab Rebase, fix tests, review & update CHANGELOG fc25086 [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection
* 3.4: Update PR template
* 4.4: Update PR template bumped Symfony version to 4.4.5 updated VERSION for 4.4.4 updated CHANGELOG for 4.4.4
* 5.0: Update PR template bumped Symfony version to 5.0.5 updated VERSION for 5.0.4 updated CHANGELOG for 5.0.4 bumped Symfony version to 4.4.5 updated VERSION for 4.4.4 updated CHANGELOG for 4.4.4
* 5.0: [Bridge/PhpUnit] dont conflict with phpunit 4.8
…e mocking http client (Koc) This PR was merged into the 5.1-dev branch. Discussion ---------- [HttpClient] Allow pass array of callable to the mocking http client | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | not yet For the now MockHttpClient allows pass closure as response factory. It useful for tests to perform assertions that expected request was sent. But If we are sending multiple sequental requests then it became a little bit tricky to perform assertions: ```php <?php $requestIndex = 0; $expectedRequest = function ($method, $url, $options) use (&$requestIndex) { switch (++$requestIndex) { case 1: $this->assertSame('GET', $method); $this->assertSame('https://example.com/api/v1/customer', $url); return new MockResponse(CustomerFixture::CUSTOMER_RESPONSE); case 2: $this->assertSame('POST', $method); $this->assertSame('https://example.com/api/v1/customer/1/products', $url); $this->assertJsonStringEqualsJsonFile(CustomerFixture::CUSTOMER_PRODUCT_PAYLOAD, $options['json']); return new MockResponse(); default: throw new \InvalidArgumentException('Too much requests'); } }; $client = new MockHttpClient($expectedRequest); static::$container->set('http_client.example', $client); $commandTester->execute(['--since' => '2019-01-01 00:05:00', '--until' => '2019-01-01 00:35:00']); $this->assertSame(2, $requestIndex, 'All expected requests was sent.'); ``` This PR introduces possibility to define multiple callable response factories and `getSentRequestsCount` method to make sure that each factory was called: ```php <?php $expectedRequests = [ function ($method, $url, $options) { $this->assertSame('GET', $method); $this->assertSame('https://example.com/api/v1/customer', $url); return new MockResponse(CustomerFixture::CUSTOMER_RESPONSE); }, function ($method, $url, $options) { $this->assertSame('POST', $method); $this->assertSame('https://example.com/api/v1/customer/1/products', $url); $this->assertJsonStringEqualsJsonFile(CustomerFixture::CUSTOMER_PRODUCT_PAYLOAD, $options['json']); return new MockResponse(); }, ]; $client = new MockHttpClient($expectedRequest); static::$container->set('http_client.example', $client); $commandTester->execute(['--since' => '2019-01-01 00:05:00', '--until' => '2019-01-01 00:35:00']); $this->assertSame(2, $client->getSentRequestsCount(), 'All expected requests was sent.'); ``` Also it adds a lot of tests. Commits ------- a36797d Allow pass array of callable to the mocking http client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, here are some remaining comments. I invite you to open the PR on the official repo now. Thanks!
use Psr\Log\LoggerInterface; | ||
use Symfony\Component\HttpClient\Exception\InvalidArgumentException; | ||
use Symfony\Component\HttpClient\Exception\TransportException; | ||
use Symfony\Component\HttpFoundation\IpUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should throw a LogicException when the class is missing, to invite to install http-foundation (see similar exceptions in the source code - look for "composer require")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did what CachingHttpClient does, except that I removed version information from exception string, because IpUtils was introduced on version 2.2. Is that OK or should I use ^4.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CachingHttpClient takes a class from HttpKernel as argument so it cannot be used without the component, that's why there is no check there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<?php
public function __construct(HttpClientInterface $client, $subnets = null)
{
if (!class_exists(IpUtils::class)) {
throw new \LogicException(sprintf('Using "%s" requires that the HttpFoundation component is installed, try running "composer require symfony/http-foundation".', __CLASS__));
}
// ...
}```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this would be more consistent with existing errors:
'You can not use "%s" if the HttpFoundation component is not installed. Try running "composer require symfony/http-foundation".'
use Symfony\Contracts\HttpClient\ResponseStreamInterface; | ||
|
||
/** | ||
* Decorator that block requests to private network by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decorator that blocks requests to private networks by default.
]; | ||
|
||
/** | ||
* @var HttpClientInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed - already explicit in the signature of the constructor
/** | ||
* Subnets in CIDR notation that IpUtils will make use of. | ||
* | ||
* @var string|array|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docblock can be removed also
private $subnets; | ||
|
||
/** | ||
* Constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed
/** | ||
* Constructor. | ||
* | ||
* @param HttpClientInterface $client A HttpClientInterface instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be removed as everything is already in the signature
(null !== $subnets && IpUtils::checkIp($info['primary_ip'], $subnets)) || | ||
(null === $subnets && ( | ||
(0 < substr_count($info['primary_ip'], '.') && IpUtils::checkIp($info['primary_ip'], self::IPV4_PRIVATE_SUBNETS)) || | ||
(0 < substr_count($info['primary_ip'], ':') && IpUtils::checkIp($info['primary_ip'], self::IPV6_PRIVATE_SUBNETS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we merge both IPV4/V6 constants in one since IpUtils::checkIp()
is able to deal with both?
this would reduce the logic to
IpUtils::checkIp($info['primary_ip'], $subnets ?? self::PRIVATE_SUBNETS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation will be more simpler, but we'll lose performance by checking IPv4 on IPv6 subnets (that will return false, which is expected), but still are unnecessary checks, is that OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's OK, the check is fast compared to the network anyway.
(0 < substr_count($info['primary_ip'], ':') && IpUtils::checkIp($info['primary_ip'], self::IPV6_PRIVATE_SUBNETS)) | ||
)) | ||
) { | ||
throw new TransportException(sprintf('IP "%s" is blacklisted.', $info['primary_ip'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could give the url also:
sprintf('IP "%s" is blacklisted for "%s".', $info['primary_ip'], $info['url'])
$subnets = $this->subnets; | ||
|
||
$lastPrimaryIp = ''; | ||
$onProgress = $options['on_progress'] ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing this should allow more accurate error reporting when the progress callback is broken:
if (!\is_callable($options['on_progress'] ?? 'var_dump')) {
return $this->client->request($method, $url, $options);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, Nicolas, I didn't get this part. If we return when user pass non-callable on on_progress, then we'll never parse his IP. You mean use var_dump by default if on_progress is non-callable or is missing? Then we can securelly call $onProgress because it will always is a callable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the option is non-null but is not a callable, and error will be raised so there is nothing to do.
$lastPrimaryIp = $info['primary_ip']; | ||
} | ||
|
||
\is_callable($onProgress) && $onProgress($dlNow, $dlSize, $info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the previoulsy suggested check, this should be reduced to:
null !== $onProgress && $onProgress($dlNow, $dlSize, $info);
aa3f856
to
c5147dd
Compare
…as been collected (nicolas-grekas) This PR was merged into the 5.1-dev branch. Discussion ---------- [HttpClient] dont display any content when none has been collected | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Commits ------- 36536c9 [HttpClient] dont display any content when none has been collected
c5147dd
to
e48b6d5
Compare
e48b6d5
to
63fec80
Compare
…hen publishing a message. (jwage) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Messenger] [Amqp] Handle AMQPConnectionException when publishing a message. | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix symfony#36538 Fix symfony#48241 | License | MIT If you have a message handler that dispatches messages to another queue, you can encounter `AMQPConnectionException` with the message "Library error: a SSL error occurred" or "a socket error occurred" depending on if you are using tls or not or if you are running behind a load balancer or not. You can manually reproduce this issue by dispatching a message where the handler then dispatches another message to a different queue, then go to rabbitmq admin and close the connection manually, then dispatch another message and when the message handler goes to dispatch the other message, you will get this exception: ``` a socket error occurred #0 /vagrant/vendor/symfony/amqp-messenger/Transport/AmqpTransport.php(60): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpSender->send() #1 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(62): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpTransport->send() #2 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle() #3 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(61): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle() #4 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle() #5 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle() #6 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle() #7 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle() #8 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch() #9 /vagrant/src/Messenger/MessageBus.php(37): Symfony\Component\Messenger\TraceableMessageBus->dispatch() #10 /vagrant/vendor/symfony/mailer/Mailer.php(66): App\Messenger\MessageBus->dispatch() #11 /vagrant/src/Mailer/Mailer.php(83): Symfony\Component\Mailer\Mailer->send() #12 /vagrant/src/Mailer/Mailer.php(96): App\Mailer\Mailer->send() #13 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(118): App\Mailer\Mailer->sendEmail() #14 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(72): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->handle() #15 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(152): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->__invoke() #16 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(91): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->callHandler() #17 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(71): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->handle() #18 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle() #19 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(68): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle() #20 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle() #21 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle() #22 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle() #23 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle() #24 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch() #25 /vagrant/vendor/symfony/messenger/RoutableMessageBus.php(54): Symfony\Component\Messenger\TraceableMessageBus->dispatch() #26 /vagrant/vendor/symfony/messenger/Worker.php(162): Symfony\Component\Messenger\RoutableMessageBus->dispatch() #27 /vagrant/vendor/symfony/messenger/Worker.php(109): Symfony\Component\Messenger\Worker->handleMessage() #28 /vagrant/vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(238): Symfony\Component\Messenger\Worker->run() #29 /vagrant/vendor/symfony/console/Command/Command.php(326): Symfony\Component\Messenger\Command\ConsumeMessagesCommand->execute() #30 /vagrant/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run() #31 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(126): Symfony\Component\Console\Application->doRunCommand() #32 /vagrant/vendor/symfony/console/Application.php(324): Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() #33 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(80): Symfony\Component\Console\Application->doRun() #34 /vagrant/vendor/symfony/console/Application.php(175): Symfony\Bundle\FrameworkBundle\Console\Application->doRun() #35 /vagrant/vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php(49): Symfony\Component\Console\Application->run() #36 /vagrant/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() #37 /vagrant/bin/console(11): require_once('...') #38 {main} ``` TODO: - [x] Add test for retry logic when publishing messages Commits ------- f123370 [Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.
Bringing symfony/symfony's branch master and my commit...