ui icon indicating copy to clipboard operation
ui copied to clipboard

Feature - Add Psr7

Open abbadon1334 opened this issue 3 years ago • 8 comments

  • [x] add Psr7 Response - nyholm/psr7
  • [x] add Response Emitter - laminas/laminas-httphandlerrunner
  • [ ] ~Rework tests to use directly App->getResponse() : ResponseInterface~
  • [x] Emit response SSE
  • [x] Add Test response vs output
  • [x] Add Psr7 Request
  • [ ] Update Atk4\Ui\Persistence\Post

abbadon1334 avatar Nov 25 '21 22:11 abbadon1334

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 avatar Nov 27 '21 08:11 abbadon1334

https://github.com/atk4/ui/blob/beb149f3e15ffdb681df9964cd5f48ed85971851/src/App.php#L1119 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 avatar Nov 27 '21 08:11 abbadon1334

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 https://github.com/atk4/ui/pull/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.

abbadon1334 avatar Nov 27 '21 08:11 abbadon1334

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 avatar Nov 27 '21 12:11 mvorisek

https://github.com/atk4/ui/blob/beb149f3e15ffdb681df9964cd5f48ed85971851/src/App.php#L1119

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

mvorisek avatar Nov 27 '21 12:11 mvorisek

some Tests are failing at the moment due to github problems, but I think all tests are successful.

abbadon1334 avatar Nov 27 '21 22:11 abbadon1334

the next step is to replace all direct calls to $_GET and $_POST. for ex : https://github.com/atk4/ui/blob/09ec6e11c3c709cb2e5ac78b71ceee65d836b3d0/src/Accordion.php#L151

can be replaced with : $this->getApp()->issetRequestQueryParam('__atk-dyn-section') and this is ok, the problem is where is no reference to App, for ex. : https://github.com/atk4/ui/blob/09ec6e11c3c709cb2e5ac78b71ceee65d836b3d0/src/Persistence/Post.php#L19

which cannot be changed with : $this->getApp()->getRequestFormParam($field) because Persistence has no reference to App

IMHO every external param must be recovered from the request object, getting request vars directly from global $_GET or $_POST is wrong or at least makes usage of sanitizer or intrusion check middlewares useless.

@mvorisek what is your opinion on this?

abbadon1334 avatar Dec 19 '21 17:12 abbadon1334

IMHO every external param must be recovered from the request object, getting request vars directly from global $_GET or $_POST is wrong or at least makes usage of sanitizer or intrusion check middlewares useless.

@mvorisek what is your opinion on this?

Definitely, maybe even unset such env. vars to make sure, all accesses are done thru the App/PSR request interface

mvorisek avatar Feb 18 '22 20:02 mvorisek

Thank you @abbadon1334 ❤️

mvorisek avatar Feb 28 '23 18:02 mvorisek