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

Impl. response using PSR-7 interface #1706

Merged
merged 98 commits into from
Feb 28, 2023
Merged

Impl. response using PSR-7 interface #1706

merged 98 commits into from
Feb 28, 2023

Conversation

abbadon1334
Copy link
Collaborator

@abbadon1334 abbadon1334 commented Nov 25, 2021

App::outputXxx/terminateXxx methods not longer accepts $headers as 2nd param, call $app->setResponseHeader() before it if needed

src/App.php Outdated Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
Copy link
Member

@mvorisek mvorisek left a comment

Choose a reason for hiding this comment

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

request must be handled as PSR request as well

laminas dep should be not needed if this translates to a few echo/header/http_response_code lines

@abbadon1334
Copy link
Collaborator Author

request must be handled as PSR request as well

laminas dep should be not needed if this translates to a few echo/header/http_response_code lines

I replace with code, but because of the small difference in performance, I always prefer the ready-made solutions, which are already tested in almost every situation, even those we don't know at the moment.

@abbadon1334
Copy link
Collaborator Author

ui/src/App.php

Line 1119 in beb149f

exit(1);

Are you sure that this cannot be changed to an Exception or other ways to not force exit the process?
My fear is that if you use Atk4 in a routing/handler contest, that exit in place of an Error will stop the flow without an output, without running post route middleware or logging of the error, without considering that to avoid that line you must override via copy/paste the whole method just to remove it.

@abbadon1334
Copy link
Collaborator Author

request must be handled as PSR request as well

laminas dep should be not needed if this translates to a few echo/header/http_response_code lines

I see the PR #1671 which remove/assert that there is no global sticky.
i think in App __construct, if not injected via $defaults['request'], we create a Request using nyholm/psr7-server and all sticky + $_GET/$_POST/$_FILES that are already present somewhere in the app must point to App->request, what you think?

But before this i need to understand why demos, except for SSE, works perfect, but not in tests.

@mvorisek
Copy link
Member

request must be handled as PSR request as well
laminas dep should be not needed if this translates to a few echo/header/http_response_code lines

I replace with code, but because of the small difference in performance, I always prefer the ready-made solutions, which are already tested in almost every situation, even those we don't know at the moment.

replacing literaly no more than several lines and doubling the non-dev dependencies, is IMHO really not helpful

@mvorisek
Copy link
Member

ui/src/App.php

Line 1119 in beb149f

exit(1);

Are you sure that this cannot be changed to an Exception or other ways to not force exit the process?
My fear is that if you use Atk4 in a routing/handler contest, that exit in place of an Error will stop the flow without an output, without running post route middleware or logging of the error, without considering that to avoid that line you must override via copy/paste the whole method just to remove it.

can be put into a protected method that can be overriden with a custom exit handler, but as described, it is too late to have a full control of the output as partly sent already

src/App.php Outdated Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
@mvorisek mvorisek force-pushed the feature/add-psr7 branch 5 times, most recently from 63706b1 to 45d7382 Compare February 28, 2023 10:31
@mvorisek mvorisek marked this pull request as ready for review February 28, 2023 18:04
@mvorisek mvorisek changed the title Impl. request/response using PSR7 Impl. response using PSR-7 interface Feb 28, 2023
Comment on lines 344 to 376
public function issetRequestGetParam(string $key): bool
{
return isset($this->request->getQueryParams()[$key]);
}

/**
* Return $_GET param by key or null if not exists.
*
* @return mixed
*/
public function getRequestGetParam(string $key)
{
return $this->request->getQueryParams()[$key] ?? null;
}

/**
* Return whole $_POST data.
*/
public function getRequestPostParams(): array
{
return $this->request->getParsedBody() ?? [];
}

/**
* Return $_POST param by key or null if not exists.
*
* @return mixed
*/
public function getRequestPostParam(string $key)
{
return $this->request->getParsedBody()[$key] ?? null;
}

Copy link
Member

Choose a reason for hiding this comment

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

to be reintroduced in #2009

@mvorisek mvorisek merged commit 4e5fc69 into develop Feb 28, 2023
@mvorisek mvorisek deleted the feature/add-psr7 branch February 28, 2023 18:32
@mvorisek
Copy link
Member

Thank you @abbadon1334 ❤️

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

Successfully merging this pull request may close these issues.

2 participants