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

Introducing the low-level API #324

Merged
merged 11 commits into from
Oct 4, 2023
Merged

Conversation

Art4
Copy link
Collaborator

@Art4 Art4 commented Sep 20, 2023

Hey 👋

This issue is more about a communication strategy than a technical issue.

I was thinking about #146 to implement a switch for JSON/XML responses.

The problem

Example: To change a project I run this code:

$response = $client->getApi('project')->update(1, [
    'name' => 'renamed project',
    'custom_fields' => [
        [
            'id' => 123,
            'name' => 'cf_name',
            'field_format' => 'string',
            'value' => [1, 2, 3],
        ],
    ],
]);

Because Redmine\Api\Project::update() uses the XML endpoint internally $response is a SimpleXMLElement instance and the response body contains XML as well, but I would like to have JSON:

var_dump($client->getLastResponseBody());
// (string): "<?xml version="1.0" encoding="UTF-8"?><project[...]"
// But I would like to have JSON...

Workaround 1

The current workaround is to check for SimpleXMLElement and use json_encode($response).

if ($response instanceof \SimpleXMLElement)
{
    $responseAsJsonString = json_encode($response);
}

Workaround 2

Another workaround could be to use $client->requestPut() directly to request the JSON endpoint:

$client->requestPut(
    '/projects/1.json',
    '{"project":{"name":"renamed project","custom_fields":[["id":123,"name":"cf_name","field_format":"string","value":[1,2,3]]]}}'
]);

$response = $client->getLastResponseBody();

Now $response will be a JSON string. But this is not very intuitive, because now you have to know about the correct endpoints and handle JSON data with json_encode() and json_decode() again for the request instead on the response.

New solution: Allow Serializers

So I came up with the idea to allow the public usage of the new serializer from #310 and #315. (The serializers are marked as @internal atm)

The code from the first example could be look like this:

$client->requestPut('/projects/1.json', (string) \Redmine\Serializer\JsonSerializer::createFromArray(['project' => [
    'name' => 'renamed project',
    'custom_fields' => [
        [
            'id' => 123,
            'name' => 'cf_name',
            'field_format' => 'string',
            'value' => [1, 2, 3],
        ],
    ],
]]));

$response = $client->getLastResponseBody();

low-level API: $client->request*() + Serializers

To switch between JSON or XML request/response by using the correct endpoint is already possible but requires more knowledge about the Redmine API, converting and sanitizing XML/JSON data and handling query parameters. The Serializer can simplify this.

For better communication I propose to call this the low-level API.

In this PR I started to point this out.

mid-level API: $client->getApi()

In contrary the "normal" way of calling $client->getApi('...') will be called the mid-level API. The mid-level API is build on top of the low-level API.

The mid-level API remains the recommended way to use php-redmine-api, but for special cases or missing features like #192 or #263 we can point to the low-level API as workaround.

@Art4 Art4 requested a review from kbsali September 20, 2023 13:22
@Art4 Art4 self-assigned this Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8665629) 93.97% compared to head (bcabc70) 95.32%.

Additional details and impacted files
@@             Coverage Diff              @@
##               v2.x     #324      +/-   ##
============================================
+ Coverage     93.97%   95.32%   +1.35%     
- Complexity      362      365       +3     
============================================
  Files            27       27              
  Lines          1128     1134       +6     
============================================
+ Hits           1060     1081      +21     
+ Misses           68       53      -15     
Files Coverage Δ
src/Redmine/Api/AbstractApi.php 82.75% <100.00%> (+8.62%) ⬆️
src/Redmine/Api/Project.php 100.00% <100.00%> (+9.09%) ⬆️
src/Redmine/Serializer/JsonSerializer.php 100.00% <100.00%> (ø)
src/Redmine/Serializer/PathSerializer.php 100.00% <100.00%> (ø)
src/Redmine/Serializer/XmlSerializer.php 96.29% <100.00%> (+0.06%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Art4 Art4 marked this pull request as ready for review September 27, 2023 11:02
@Art4 Art4 added this to the v2.3.0 milestone Sep 28, 2023
@Art4 Art4 mentioned this pull request Sep 28, 2023
@Art4
Copy link
Collaborator Author

Art4 commented Sep 28, 2023

@kbsali Any thoughts?

Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

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

👍

@Art4 Art4 merged commit 2b7cfcd into kbsali:v2.x Oct 4, 2023
7 checks passed
@Art4 Art4 deleted the documentation-for-low-level-api branch December 18, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants