atk4 / filestore

Implements integration between ATK UI and FlySystem
https://agiletoolkit.org/
MIT License
9 stars 8 forks source link

Adjust to atk4/ui PSR-7 response #32

Closed mvorisek closed 1 year ago

mvorisek commented 1 year ago

related with https://github.com/atk4/ui/pull/1706

abbadon1334 commented 1 year ago

Pull Request Draft: Stream Handling Improvement

This pull request aims to remove the problematic exit statement in a cleaner way by implementing better stream handling. The proposed changes include adding a new condition to App::terminate and modifying the Helper::output method.

Changes to App::terminate

Add the following if condition at the beginning of the method:

if ($output instanceof StreamInterface) { // if is a stream, emit directly
    $this->response = $this->response->withBody($output);
    $this->emitResponse();

} elseif ($type === 'application/json') {

Changes to Helper::output

Modify the method with the following code:

$resource = $model->flysystem->readStream($path);
$factory = new \Nyholm\Psr7\Factory\Psr17Factory();
$app->terminate($factory->createStreamFromResource($resource));

Rationale

Basing the output guessing on headers is not suitable for streams. For example, if a file with application/json content needs to be streamed, it will pass the current first condition and proceed to App::outputResponseJson rather than being treated as a stream. Furthermore, during the processing of App::terminate, the JSON will be added with a new property portals (which creates a useless property for those using ATK to create API services).

Additionally, it is incorrect to pass the stream to the App::outputResponse method, where a stream is created and must be a string.

Implementing the proposed changes will address these issues and provide a cleaner solution.

Please let me know your thoughts on this proposal, @mvorisek and @DarkSide666. so @DarkSide666 or I can complete #33 and open a new PR on Atk/Ui

DarkSide666 commented 1 year ago

I like that.

Instead of $stream = (new \Nyholm\Psr7\Factory\Psr17Factory())->createStreamFromResource($resource) I was using $stream = \Nyholm\Psr7\Stream::create($resource) but I guess that's the same thing at the end.

mvorisek commented 1 year ago

Changes to App::terminate

The proposed if ($output instanceof StreamInterface) { placed as the first if looks reasonable.

https://github.com/atk4/ui/blob/4e5fc69160677/src/App.php#L403 Content-Type will still be asserted to be set.

The if should look more like:

if ($output instanceof StreamInterface) {
    $this->response = $this->response->withBody($output);
    $this->outputResponse('');

} elseif ($type === 'application/json') {

to trigger other assertions like unexpected output and/or headers already set in https://github.com/atk4/ui/blob/4e5fc69160677/src/App.php#L1109-L1121.

DarkSide666 commented 1 year ago

OK, but I'm not sure about this part https://github.com/atk4/ui/blob/4e5fc69160677/src/App.php#L1131. I think it will overwrite Stream with empty data then isn't it? Maybe it should be like this then?

if ($data !== '') {
    $this->response->getBody()->write($data);
}
mvorisek commented 1 year ago

I expect the (empty string) data appended.

DarkSide666 commented 1 year ago

Proably this PR should be closed in favor of https://github.com/atk4/filestore/pull/33

mvorisek commented 1 year ago

Proably this PR should be closed in favor of #33

I will finish this PR, please rebase your #33 on top of this PR.