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

Support for request body in CastleDownload (fpHttpClient) #558

Merged

Conversation

phomm
Copy link
Contributor

@phomm phomm commented Dec 23, 2023

@michaliskambi , please point to the parts which are not covered and should be improved

@phomm
Copy link
Contributor Author

phomm commented Dec 28, 2023

FYI, had to manually update yml files, as my git tool didn't have ability to push them (some token issue), but in result I merged all changes from latest cge-master changes

@phomm phomm changed the title Implementation of support for request body in CastleDownload (fpHttpClient) Support for request body in CastleDownload (fpHttpClient) Jan 17, 2024
@phomm
Copy link
Contributor Author

phomm commented Jan 23, 2024

Merged latest master changes

@michaliskambi
Copy link
Member

Thanks for synching with master. I hope to finally get to addressing and merging this PR (and some others), now that I finally merged to CGE the big work (>400 commits) from delphi-linux branch :)

phomm and others added 3 commits January 26, 2024 18:00
… improvements

- Added example examples/network/put_data/put_data.dpr

- Let HttpRequestBody be used by all requests, not only PUT or POST.

    Reason: FPC FpHttpClient sources comments say it sends it also on DELETE and OPTIONS, and actual implementation just sends it always (TFPCustomHTTPClient.SendRequest). And that's probably better -- let the user decide when sending it makes sense, technically it's a stream that can be sent with any request.

    (Of course, the practical / most common usage is to use it with PUT, to provide data to PUT. Or with POST, if normal key-value POST parameters are not flexible enough.)

- Improved a bit the docs:

    - Mention WriteStr(Download.HttpRequestBody, 'My content')
    - Point to example examples/network/put_data/put_data.dpr

- Addressed the fact that HttpBody = nil had special meaning, but also it was created on-demand. This may be surprising, e.g. doing

    ```delphi
    D := TCastleDownload.Create;
    Writeln(D.HttpBody.Count);
    ```

    actually changes what the request is doing (`D.HttpPostData` will be ignored), even though the line `Writeln(D.HttpBody.Count)` looks innocent.

    I decided to change the logic: let it be used only if non-empty, i.e. non-nil and Count <> 0.

- Rename `HttpBody` to longer but more obvious `HttpRequestBody`.

- Since we create it on our side, we can expose it as TMemoryStream , not abstract TStream.
@michaliskambi
Copy link
Member

Thank you and sorry for delay in addressing this!

I made tests and did a few modifications. See my commit with details in c8a660c . Practically, the most important is that I renamed it to HttpRequestBody and added an example. I also made really simple test in PHP to receive PUT and make sure it is OK -- https://github.com/castle-engine/cge-www/blob/master/htdocs/miscella/test_put.php ,

Merging!

@michaliskambi michaliskambi merged commit 7836f26 into castle-engine:master Jan 29, 2024
16 of 17 checks passed
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.

None yet

2 participants