Remove calls to exit functions and cleanups #790

abbadon1334 · 2019-08-02T17:00:00Z

i removed calls to php exit function, that create many problem and not allow to include the atk in a any other framework that use PSR-7 with request -> response flow.

what i have done :

i removed all "generalized" try catch, any Error or Exception not strictly related to the flow must be throw upper to the App->catchException.
add some throw to methods.
simplify methods when i see some old syntax.
i try to commit on every change to have comments on the work in progress.
PHPUnit on my PC pass all tests
Manual testing to be sure all works correctly

I had only one problem in : https://github.com/abbadon1334/ui/blob/18ce128a68ad1785f0d33e09f584bc1d77992de8/src/Form.php#L592

Where in Form.php we return "Direct output" as an error, and i normalized it to Exception.

just to give no error and be happy.

codecov · 2019-08-02T17:08:04Z

Codecov Report

Merging #790 into develop will increase coverage by 0.49%.
The diff coverage is 85.71%.

@@              Coverage Diff              @@
##             develop     #790      +/-   ##
=============================================
+ Coverage      71.17%   71.66%   +0.49%     
+ Complexity      2246     2240       -6     
=============================================
  Files            119      119              
  Lines           5328     5311      -17     
=============================================
+ Hits            3792     3806      +14     
+ Misses          1536     1505      -31

Impacted Files	Coverage Δ	Complexity Δ
src/Form.php	`72.6% <ø> (-3.43%)`	`66 <0> (-2)`
src/jQuery.php	`71.42% <ø> (ø)`	`3 <0> (ø)`	⬇️
src/View.php	`88.75% <100%> (+3.24%)`	`138 <6> (-4)`	⬇️
src/App.php	`80% <62.5%> (+8.4%)`	`117 <3> (ø)`	⬇️
src/TableColumn/Status.php	`82.85% <0%> (+2.85%)`	`12% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac79ff6...1324961. Read the comment docs.

codecov · 2019-08-02T17:08:05Z

Codecov Report

Merging #790 into develop will increase coverage by 5.25%.
The diff coverage is 82.5%.

@@              Coverage Diff              @@
##             develop     #790      +/-   ##
=============================================
+ Coverage      71.26%   76.51%   +5.25%     
- Complexity      2242     2250       +8     
=============================================
  Files            119      119              
  Lines           5325     5336      +11     
=============================================
+ Hits            3795     4083     +288     
+ Misses          1530     1253     -277

Impacted Files	Coverage Δ	Complexity Δ
src/jQuery.php	`71.42% <ø> (ø)`	`3 <0> (ø)`	⬇️
src/Form.php	`74.65% <ø> (-3.43%)`	`66 <0> (-2)`
src/View.php	`90% <100%> (+4.49%)`	`138 <6> (-4)`	⬇️
src/jsSSE.php	`94.82% <75%> (+18.04%)`	`23 <0> (ø)`	⬇️
src/App.php	`82.81% <79.68%> (+11.22%)`	`131 <11> (+14)`	⬆️
src/Menu.php	`94.73% <0%> (-5.27%)`	`25% <0%> (ø)`
src/TableColumn/Multiformat.php	`91.89% <0%> (-2.71%)`	`12% <0%> (ø)`
src/Wizard.php	`84.28% <0%> (-1.43%)`	`23% <0%> (ø)`
src/Table.php	`84.61% <0%> (ø)`	`110% <0%> (ø)`	⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56e6843...480301d. Read the comment docs.

romaninsh · 2019-08-02T20:51:18Z

src/App.php

+        // catch Exit Exception and return
+        if ($exception instanceof ExitApplicationException) {
+            return true;
+        }


clever approach to avoid error.

But it's different to what we dicsussed and i think this can cause problem if you run app with "catch_exception = false".

Also i think it will create some insanity if used with logger (which normally log all unhandled exceptions)

Best to implement the property App::call_exit = true , which can be modified to "false" if exception is required.

Need to be App to stop execution.

just try to solve issue on travis for Callbacks TEST

they are caught in set_exception_handler or custom catch

add a TODO about checking this because multiple sent, may be the cause is the underlying developer code

use the built in server i use in local testing

abbadon1334 · 2019-08-05T11:28:51Z

i try to added to request/response check for demos and coverage, the new actions from demos/actions.php

Local it works perfectly, on travis if i call the url POST for confirm in download popup action or the url POST for submit the form in edit popup for record 1, i get an error of record not found, which is wrong because i call first the action to importFromFileSystem which will populate the database.

I don't understand what is wrong on travis.

Probably i will divide this PR in smaller PR

abbadon1334 · 2019-08-05T11:59:26Z

demos/init.php

+    'catch_exceptions' => isset($_GET['APP_CATCH_EXCEPTIONS']) && $_GET['APP_CATCH_EXCEPTIONS'] == 0 ? false : true,
+]);
+
+if ($app->call_exit !== true) {


can be tested for both exits ( call_exit=true and call_exit=false)

abbadon1334 · 2019-08-05T12:01:09Z

src/App.php

-        $l->initLayout('Centered');
+        // just replace layout to avoid any extended App->_construct problems
+        // it will maintain everything as in the original app StickyGet, logger, Events
+        $this->html = null;


in place of creating a new app for exception, why not destroy the render tree making $app->html = null
and reuse the actual app, all $defaults will be respected.

abbadon1334 · 2019-08-05T12:03:15Z

src/App.php

-            echo $output;
+            if ($this->isJsonRequest()) {
+                $this->outputResponseJSON($output);
+            } else {


any response from atk4 is text/html, i made 2 function to respond with JSON or HTML headers
i have also made a outputResponse with custom headers for any special needs (SEE?)

abbadon1334 · 2019-08-05T12:04:56Z

src/App.php

+            }
+            echo $this->html->template->render();
+        } catch (ExitApplicationException $e) {
+            $is_exit_exception = true;


wrap all App->run i a try catch that catch only exit application exception and call exit app if raised

abbadon1334 · 2019-08-05T12:09:54Z

src/Form.php

@@ -595,7 +595,7 @@ public function ajaxSubmit()
                ob_start();
                $this->loadPOST();
                $response = $this->hook('submit');
-                $output = ob_get_contents();
+                $output = ob_get_clean();


i raise exception to the upper level to be caught by the global exception handler

abbadon1334 · 2019-08-05T12:16:21Z

src/View.php

-            } else {
-                $l->add(new self(['ui' => 'message', get_class($exception).': '.$exception->getMessage(), 'error']));
-            }
+        $this->renderAll();


raise exception to upper level
we already have exception handler that format correctly the exception, if an exception happen here will broke the normal flow of the whole application.

abbadon1334 · 2019-08-05T12:16:53Z

src/View.php

     */
-    public function stickyGet($name, $newValue = null)
+    public function stickyGet($name, $newValue = null) : ?string


just simplify this

abbadon1334 · 2019-08-05T12:17:54Z

tests/BuiltInWebServerAbstract.php

+use Psr\Http\Message\ResponseInterface;
+use Symfony\Component\Process\Process;
+
+abstract class BuiltInWebServerAbstract extends TestCase


add alternative base Test class to test real GET POST request using built in server

abbadon1334 · 2019-08-05T12:19:06Z

tools/test.sh

@@ -1,7 +1,7 @@
 #!/bin/bash

 echo "Setting up coverage logging";
-mkdir coverage
+mkdir -p coverage


add -p to mkdir to avoid errors on call to mkdir
-p, --parents no error if existing, make parent directories as needed

abbadon1334 · 2019-08-05T12:21:14Z

tests/BuiltInWebServerAbstract.php

+
+    public static function setUpBeforeClass()
+    {
+        if (!file_exists(getcwd().'/coverage/')) {


to get coverage on tests run with built in server i use the same idea that we already use with selenium, include file coverage.php and collect .cov files

abbadon1334 added 12 commits August 2, 2019 15:52

Remove call to function exit() from App->callExit

86d8f12

add file in demo to test exception and error Throwing

51c655d

i think this is better because name is not public

cab760d

Refactor Exception catch

1a99ff1

intellisense PHPStorm - add arguments jQuery->bind

200198f

just to give no error and be happy.

remove try/catch from renderJSON => send Excep. to App->caughtException

7b243b8

removed define from Test

943edfc

clean UP

d67870a

simplify method stickyGet

2b53049

Add some comment to callExit method

067ec0b

StyleCi

e9ca95b

StyleCi 2

1324961

abbadon1334 requested review from romaninsh, ibelar, DarkSide666 and gowrav-vishwakarma and removed request for romaninsh August 2, 2019 17:15

romaninsh requested changes Aug 2, 2019

View reviewed changes

abbadon1334 added 9 commits August 3, 2019 12:35

remove extra variable from phpunit.xml

60a33e8

ADD composer guzzle|process dev - for test with PHPBuiltinServer

5af9c09

Create Abstract Test for BuiltinServer with coverage

6b1a6d3

Refactor DemoTest.php to use BuiltInWebServerAbstract

de7aa50

add $app->call_exit

105148e

add catch of exception and rethrow to upper level of exceptions.

6c9e5e5

Need to be App to stop execution.

add properties for testing with and without call_exit in init.php

6116962

Refactor Demo Test and duplicate to test call_exit=false

a73a541

raise exception throwing

a8e12cc

abbadon1334 added 19 commits August 4, 2019 13:37

add test for catch exception in demos/exception_test.php

af08122

StyleCI

bede367

add flush to force output? cannot recreate locally.

36de2e3

just try to solve issue on travis for Callbacks TEST

removed @flush (not working)

ecada2b

normalize response output on $app->run = $app->terminate

b4e2c6a

remove catch of Exception != ExitApplicationException in $app->run

1f8db64

they are caught in set_exception_handler or custom catch

add a check to avoid multiple send of headers

9097fee

add a TODO about checking this because multiple sent, may be the cause is the underlying developer code

Merge branch 'develop' of https://www.github.com/atk4/ui into develop

5d628d2

populate \File model to not trigger error later on test actions

60f278e

Fix : On exception not close the output buffer

0e31700

fix : Test call_exit = 1|0 (ops! i use self in place of static)

e409a8e

refactor some code and add some comment

2bc2f49

StyleCI

6d1933d

TEST : try to solve issue with demos/actions.php

8f59b03

TEST 2 : try to solve issue with demos/actions.php

276f6eb

TEST 3 : try to solve issue with demos/actions.php

a9736ef

TEST 4 : demos/actions.php why on travis don't works...

18cb741

TEST 4 : StyleCI

3f75c06

Still on actions : drop use of already started travis built in server

0bd0c16

use the built in server i use in local testing

abbadon1334 added 2 commits August 5, 2019 14:40

i commented that 2 calls that i add and let it pass PHPUNIT on travis.

dd942ad

add more detailed comment on issue for that 2 calls

480301d

abbadon1334 commented Aug 5, 2019

View reviewed changes

abbadon1334 requested a review from romaninsh August 5, 2019 12:22

abbadon1334 added the hangout agenda 🔈 Will discuss on next hangout label Aug 15, 2019

romaninsh approved these changes Aug 15, 2019

View reviewed changes

romaninsh changed the title ~~Remove calls to exit functions~~ Remove calls to exit functions and cleanups Aug 15, 2019

romaninsh merged commit 04739e7 into atk4:develop Aug 15, 2019

This was referenced Aug 22, 2019

App Terminate - Don't Exit? #506

Closed

calling terminate() will bypass CallbackLater execution #443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove calls to exit functions and cleanups #790

Remove calls to exit functions and cleanups #790

abbadon1334 commented Aug 2, 2019

codecov bot commented Aug 2, 2019

codecov bot commented Aug 2, 2019 •

edited

Loading

romaninsh Aug 2, 2019

abbadon1334 commented Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

abbadon1334 Aug 5, 2019

Remove calls to exit functions and cleanups #790

Remove calls to exit functions and cleanups #790

Conversation

abbadon1334 commented Aug 2, 2019

codecov bot commented Aug 2, 2019

Codecov Report

codecov bot commented Aug 2, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

abbadon1334 commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2019 •

edited

Loading