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.

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

Simplify signature of the low-level API #341

Closed
Art4 opened this issue Dec 28, 2023 · 1 comment · Fixed by #370
Closed

Simplify signature of the low-level API #341

Art4 opened this issue Dec 28, 2023 · 1 comment · Fixed by #370
Assignees
Milestone

Comments

@Art4
Copy link
Collaborator

Art4 commented Dec 28, 2023

At the moment we have these 7 methods to interact with the low-level API:

    /**
     * Create and send a GET request.
     */
    public function requestGet(string $path): bool;

    /**
     * Create and send a POST request.
     */
    public function requestPost(string $path, string $body): bool;

    /**
     * Create and send a PUT request.
     */
    public function requestPut(string $path, string $body): bool;

    /**
     * Create and send a DELETE request.
     */
    public function requestDelete(string $path): bool;

    /**
     * Returns status code of the last response.
     */
    public function getLastResponseStatusCode(): int;

    /**
     * Returns content type of the last response.
     */
    public function getLastResponseContentType(): string;

    /**
     * Returns the body of the last response.
     */
    public function getLastResponseBody(): string;

The first 4 methods will send a request to the server and save the response in the Client. The last 3 methods can now be used to get the response details. This has some downsides:

  1. If we send multiple requests (like we do in the mid-level API AbstractApi::retrieveData()), only the latest response will be available in the client. This leads to possible race conditions: There is no way to guarantee, that the last response in Client really belongs to our request.
  2. We have to call the client 3 times to retrieve all response data. This is a code smell and should be replaced with a small object.

Proposal

I propose to introduce a new Request containing the method, path, content type and body and a new Response interface, containing the status code, content type and body.

interface Request
{
    /**
     * Returns the http method.
     */
    public function getMethod(): string;

    /**
     * Returns the path with optional attached query string.
     */
    public function getPath(): string;

    /**
     * Returns content type.
     */
    public function getContentType(): string;

    /**
     * Returns the body content.
     */
    public function getContent(): string;
}
interface Response
{
    /**
     * Returns status code.
     */
    public function getStatusCode(): int;

    /**
     * Returns content type.
     */
    public function getContentType(): string;

    /**
     * Returns the body content.
     */
    public function getContent(): string;
}

The client methods could then simplified into one method.

public function request(Request $request): Response;
@Art4
Copy link
Collaborator Author

Art4 commented Jan 19, 2024

Another point that I would like to improve is the "content type guessing" based on the path in the clients. In NativeCurlClient and Psr18Client we have the following code to determine the content type:

        if ($this->isUploadCall($path)) { // (false !== strpos($path, '/uploads.json')) || (false !== strpos($path, '/uploads.xml'))
            $httpHeaders[] = 'Content-Type: application/octet-stream';
        } elseif ('json' === substr($tmp['path'], -4)) {
            $httpHeaders[] = 'Content-Type: application/json';
        } elseif ('xml' === substr($tmp['path'], -3)) {
            $httpHeaders[] = 'Content-Type: text/xml';
        }

This method is very speculative and uncertain. The client should not make any assumptions about the request.

Instead we should add a way to provide the content type via the request() method. It makes sense to bundle the 4 parameters (method, path, content-type, content) in a simplified request interface that is compatible to PSR-7. I will update have updated the proposal.

Usage of the low-level API could then work like this:

$response = $this->getHttpClient()->request(XmlRequest::put(
    '/issues/' . $id . '.xml',
    ['issue' => $params],
));

$array = XmlSerializer::createFromString($response->getContent())->toArray();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant