Closed DarkGhostHunter closed 2 years ago
gdespirito
commented
3 years ago Guzzle 7.0 no soporta PHP 7.0.
Si bien el SDK instala por defecto Guzzle (5, 6 o 7), puedes perfectamente crear tu propio HttpClient y pasarlo en el constructor. Falta documentarlo mejor, pero solo necesitas crear un HttpClient que implemente HttpClientInterface que solo te pide un método request. Dentro, puedes hacerlo como quieras mientras devuelva un json de respuesta (o una excepción si algo falla)
DarkGhostHunter
commented
3 years ago Guzzle 7.0 no soporta PHP 7.0.
Ni PHP soporta PHP 7.0 ya.
Si bien el SDK instala por defecto Guzzle (5, 6 o 7), puedes perfectamente crear tu propio HttpClient y pasarlo en el constructor. Falta documentarlo mejor, pero solo necesitas crear un
HttpClientque implementeHttpClientInterfaceque solo te pide un método request. Dentro, puedes hacerlo como quieras mientras devuelva un json de respuesta (o una excepción si algo falla)
Creo que esa interfaz está de más considerando que el estándar PSR-18 ya lo trae, aunque eso significa crear el ServerRequest uno mismo (de hecho, yo y Guzzle usamos Nyholm). Tampoco es tanto atado, pero como integrador es más fácil:
que
gdespirito
commented
3 years ago Quizás podríamos dar soporte solo de PHP 7.1 en adelante. Es un GRAN cambio, pero si existen buenas razones para hacerlo, podría ser. De momento no veo que sea un GRAN razón, porque se puede hacer práctivamente igual, solo que creando una clase que implemente la interfaz. Además, esta forma te permite personalizar aún más el flujo, como usar un cleinte HTTP que no implemente PSR-18 (curl quizás).
DarkGhostHunter
commented
3 years ago Se podría usar una interfaz personalizada, sin embargo, a diferencia del PSR cae a cargo el desarrollador, mientras que con el PSR, el mensaje está listo.
También podía ser que aceptara ambos (el PSR y la interfaz propia). A la hora de enviar el mensaje, si es PSR se construye el mensaje, y si es la interfaz propia, se pasa en bruto.
gdespirito
commented
3 years ago Mientras mantengamos soporte para PHP 7.0, me parece bien.
DarkGhostHunter
commented
3 years ago Mi voto es para soportar ambas. El único drama sería introducir Nyholm para crear el Request, pero eso se hace una vez no más.
gdespirito
commented
3 years ago Pero eso requiere PHP 7.1 , por lo poco que leí en el link de Nyholm ¿no?
DarkGhostHunter
commented
3 years ago Yep, pero la 0.23 no y creo, no estoy seguro, que todavía es compatible.
mnavarrocarter
commented
3 years ago Estoy muy de acuerdo con el objetivo de eliminar la dependencia en Guzzle. Pero creo que deberiamos considerar lo siguiente:
Lo que necesitamos es la habilidad de soportar multiples clientes http. Es un caso perfecto para hacer es nuestro propio contrato de un cliente HTTP simplificado. Bastara con pasar un metodo, una url, un array de parametros y un array de headers. Como respuesta devuelve un array. Ninguna implementacion especifica deberia derramarse de nuestro cliente. Algo como esto:
/**
* Interface HttpService is created as a way to abstract the making of
* HTTP Requests.
*/
interface HttpService
{
/**
* Makes an Http Request and returns a payload as a response.
*
* @throws ConnectionError When connection could not be established
* @throws ProtocolError When the server returns an error response in the 400 or 500 range
*/
public function request(string $method, string $url, array $params = [], array $headers = []): array;
}Luego, implementamos ese contrato usando PSR-17 y PSR-18 (no tenemos que traer Nyholm para hacer la request, para eso estan las factories de PSR-17).
/**
* Class FigPsrHttpService.
*/
final class FigPsrHttpService implements HttpService
{
private ClientInterface $client;
private RequestFactoryInterface $factory;
/**
* Psr7Request constructor.
*/
public function __construct(ClientInterface $client, RequestFactoryInterface $factory)
{
$this->client = $client;
$this->factory = $factory;
}
/**
* @throws ProtocolError
* @throws ConnectionError
*/
public function request(string $method, string $url, array $params = [], array $headers = []): array
{
$request = $this->createRequest($method, $url, $params, $headers);
try {
$response = $this->client->sendRequest($request);
} catch (ClientExceptionInterface $e) {
throw new ConnectionError('Failed to connect to '.$url, 0, $e);
}
try {
$payload = json_decode((string) $response->getBody(), true, 512, JSON_THROW_ON_ERROR);
} catch (JsonException $e) {
$payload = [];
}
$isError = $response->getStatusCode() >= 400;
if ($isError) {
$errorMsg = 'Unknown error';
if (is_array($payload)) {
$errorMsg = $payload['error_message'] ?? 'Unknown error';
}
throw new ProtocolError(sprintf('Error %s returned from Transbank: %s', $response->getStatusCode(), $errorMsg), $response->getStatusCode());
}
return $payload;
}
/**
* @throws ProtocolError
*/
private function createRequest(string $method, string $url, array $params = [], array $headers = []): RequestInterface
{
$request = $this->factory->createRequest($method, $url);
if ('GET' === $method) {
$request = $request->withUri($request->getUri()->withQuery(http_build_query($params)));
}
if ('GET' !== $method) {
try {
$request->getBody()->write(json_encode($params, JSON_THROW_ON_ERROR));
} catch (JsonException $e) {
throw new ProtocolError('Could not encode $params into json', 0, $e);
}
}
foreach ($headers as $header => $value) {
$request = $request->withHeader($header, $value);
}
return $request;
}
}Podriamos tener implementaciones que no requieran clientes PSR. De hecho, una que use un simple file_get_contents podria ser una implementación. Podriamos hacer una para el Cliente HTTP de Symfony e incluso para cualquier otro futuro cliente HTTP que pueda salir. Solo lo tenemos que adaptar a ese contrato, y hacer que los servicios usen el HttpService como dependencia injectada por el constructor.
mnavarrocarter
commented
3 years ago Estuve tirando algunas lineas de código y asi es como se veria esto en el create transaction de WebpayPlus, como ejemplo:
final class HttpTransactionService implements TransactionService
{
private const SERVICE_URL = '/rswebpaytransaction/api/webpay/v1.2/transactions';
private Http\Service $http;
private Environment $config;
/**
* HttpTransactionService constructor.
*/
public function __construct(Http\Service $http, Environment $config)
{
$this->http = $http;
$this->config = $config;
}
/**
* {@inheritDoc}
*/
public function create(string $orderId, string $sessionId, int $amount, string $returnUrl): CreateResult
{
try {
$data = $this->request('POST', '', [
'buy_order' => $orderId,
'session_id' => $sessionId,
'amount' => $amount,
'return_url' => $returnUrl,
]);
} catch (Http\ProtocolError $e) {
throw new UnexpectedError('An error has occurred', 0, $e);
}
return new CreateResult($data['token'], $data['url']);
}
/**
* @throws UnexpectedError
* @throws Http\ProtocolError
*/
private function request(string $method, string $path, array $params = []): array
{
$url = $this->config->getBaseUrl().self::SERVICE_URL.$path;
try {
return $this->http->request($method, $url, $params, [
'Content-Type' => 'application/json',
'Tbk-Api-Key-Id' => $this->config->getCommerceCode(),
'Tbk-Api-Key-Secret' => $this->config->getApiKey(),
]);
} catch (Http\ConnectionError $e) {
throw new UnexpectedError('Could not connect to Transbank', 0, $e);
}
}
}@mnavarrocarter esta sería la interfaz: https://github.com/TransbankDevelopers/transbank-sdk-php/blob/master/src/Contracts/RequestService.php
Justo ayer hice un PR con eso.
mnavarrocarter
commented
3 years ago Ah genial, no la habia visto.
Y cual seria el proposito de Options ahi?
Lo otro es que hay que especificar los tipos de los argumentos y lo que se devuelve, por ejemplo, un array.
gdespirito
commented
3 years ago Conocer la URL Base, y los headers.
Options->getBaseUrl() y Options->getHeaders().
gdespirito
commented
3 years ago ¿Qué hacemos con esto? Creoq ue como está ahora se puedes extender y mejorar sin hacer breaking changes. Necesito saber si es blocker como para hacer el release hoy :)
DarkGhostHunter
commented
3 years ago ¿Qué hacemos con esto? Creoq ue como está ahora se puedes extender y mejorar sin hacer breaking changes. Necesito saber si es blocker como para hacer el release hoy :)
No es blocking.
La clase final sabe qué mete, como un logger. De hecho, la clase que usa la interfaz debería tener un setLogger($logger).
ffflabs
commented
3 years ago Ojo @gonzunigad hay un error de regresión en el PR mencionado. Aparentemente el commit "arregla patpass" recibió encima un merge que no permite usar la transacción (por ejemplo porque ya no existe PatPassByWebPay::getHttpClient, y de paso quita la constsnte que indica el commercecode por defecto para el mismo servicio
gdespirito
commented
3 years ago Hola @ffflabs . ¿Que PR? Lo reviso.
ffflabs
commented
3 years ago Perdón la demora, estaba con celu.
Ya no estoy seguro en qué commit se rompió, pero puedo señalar 3 cosas:
Tanto si pasas un objeto Options como si no, un llamado a Transbank\Patpass\PatpassByWebpay\Transaction::create se cae en Transbank\Patpass\PatpassByWebpay::getCommerceCode o en $options->getCommerceCode.
PatpassByWebpay no tiene un método getCommerceCode puesto que se eliminó en ba2228374762db38a65b9 . En ese commit se modificó la clase para heredar de Utils\ConfiguresEnvironment, que solía tener el método removido.
Sin embargo , esta última clase tampoco tenía el método a esa altura, pues se había removido en a327acbcb61eecad4.
Mi teoría, como dije, es que ambos cambios eran inofensivos en sus respectivos PR, pero al mergear se generó un escenario inviable.
Como esta clase no tiene tests el problema no se detectó. Un test ultra básico que hice para verificar mi hallazgo es:
namespace Test\Webpay\PatpassByWebpay;
use PHPUnit\Framework\TestCase;
use Transbank\Patpass\PatpassByWebpay;
use Transbank\Patpass\PatpassByWebpay\Responses\TransactionCommitResponse;
use Transbank\Patpass\PatpassByWebpay\Transaction;
use Transbank\Utils\HttpClientRequestService;
use Transbank\Webpay\Exceptions\WebpayRequestException;
use Transbank\Webpay\Options;
class PatpassByWebpayTest extends TestCase
{
/**
* @var int
*/
protected $amount;
/**
* @var string
*/
protected $sessionId;
/**
* @var string
*/
protected $buyOrder;
/**
* @var string
*/
protected $cardNumber;
/**
* @var string
*/
protected $mockBaseUrl;
/**
* @var \PHPUnit\Framework\MockObject\MockObject|Options
*/
protected $optionsMock;
/**
* @var array
*/
protected $headersMock;
/**
* @var string
*/
protected $cardExpiration;
public function setBaseMocks(): void
{
$this->optionsMock = $this->createMock(Options::class);
$this->headersMock = ['header_1' => uniqid()];
$this->optionsMock->method('getApiBaseUrl')->willReturn($this->mockBaseUrl);
$this->optionsMock->method('getHeaders')->willReturn($this->headersMock);
}
protected function setUp(): void
{
$this->amount = 1000;
$this->sessionId = 'some_session_id_' . uniqid();
$this->buyOrder = '123999555';
$this->mockBaseUrl = 'http://mockurl.cl';
$this->cardNumber = '4051885600446623';
$this->cardExpiration = '12/24';
}
/**
* @test
*
* @return void
* @group broken_in_PR182
*/
public function it_creates_a_transaction_with_default_options(): void
{
$response = Transaction::create(
$this->buyOrder,
$this->sessionId,
$this->amount,
$this->mockBaseUrl,
[
'cardNumber' => $this->cardNumber,
'cardExpiratio' => $this->cardExpiration
]
);
$this->assertInstanceOf(TransactionCommitResponse::class, $response);
}
/**
* @test
*
* @return void
* @group broken_in_PR182
*/
public function it_creates_a_transaction_with_custom_options(): void
{
$this->setBaseMocks();
$response = Transaction::create(
$this->buyOrder,
$this->sessionId,
$this->amount,
$this->mockBaseUrl,
[
'cardNumber' => $this->cardNumber,
'cardExpiratio' => $this->cardExpiration
],
$this->optionsMock
);
$this->assertInstanceOf(TransactionCommitResponse::class, $response);
}
}También veo que la clase WebpayPlus ya no puede utilizarse como factoría de transacciones si se quiere pasar explícitamente el segundo parámetro. El método sigue esperando un HttpClient
Pero el constructor de la transacción (a través del trait InteractsWithWebpayApi) espera en cambio un RequestService
gdespirito
commented
3 years ago @ffflabs este PR y este otro PR solucionan el problema.
ffflabs
commented
3 years ago @mnavarrocarter @DarkGhostHunter tendría sentido en primera instancia, dado que se saque Guzzle de las dependencias, meter httplug en su lugar o al menos señalarlo en la documentación ?
Con eso no nos casamos con ningún cliente, tenemos adaptadores para una docena de clientes, incluyendo Curl y aunque muy desaconsejado, también uno para fopen y file_get_contents) y un camino de migración a PSR-18 cuando sea pertinente.
Con eso se deja la libertad de traer tu propio cliente http, y al mismo tiempo podemos construir la próxima RequestServiceInterface o la clase misma que la implementa en casa servicio de transbank edificando sobre un contrato mantenido y validado, que además saca provecho de las factorías PSR-17.
Técnicamente bastaría con establecer que el constructor de esos servicios implemente ClientInterface, pero me parece que se está estirando demasiado el chicle hacia el terreno donde los usuarios hacen tilt.
(Al principio pensé en sugerir el paquete virtual psr/http-client-implementation pero eso también es demasiado des-opinionado, creo yo.
dgIngenieria
commented
12 months ago estimados, no se si alguien puede ayudarme, instale pulgin Dokan para multivendedores , pero al instalar webpayplus, entra en conflicto , y no me deja pagar el carro de compra y se cae. cada uno funciona bien por si solo, pero al tener los dos activos entran en conflicto
hay alguna solucion ?
Fatal error: Uncaught Error: Call to undefined method GuzzleHttp\Utils::chooseHandler() in /home/bendito/public_html/wp-content/plugins/dokan-pro/vendor/guzzlehttp/guzzle/src/functions.php:61 Stack trace:
Problema
La dependencia con Guzzle está hard-coded. Es imposible usar otro cliente que no sea Guzzle. En otras plataformas, ya hay Clientes HTTP, como Amp, ReactPHP, y Swoole.
Tampoco es posible configurar Guzzle o testear el cliente.
Solución
Habilitar un Cliente HTTP estándar PSR-18 para permitir usar cualquiera.
Guzzle 7.0 ya es compatible con PSR-18, como también los mencionados anteriormente.
Como el Cliente HTTP sería necesario de instalar por separado, debería actualizarse la documentación para incluir la instalación de Guzzle.
Breaking Changes