Extracting the call handler for Spark commands from kernel. #6110

iRedds · 2022-06-11T09:28:58Z

Description
This is the concept of extracting the call to Spark commands from processing via routing. Commands are called directly, as when using the command() function. This will reduce the load on the console application in the form of Request, Response, Filters and Routes services and more.

In the beginning, the idea of creating an Application class was considered, from which the CodeIniter and Console classes would be inherited. This was necessary to initialize the application. But due to the fact that, at the request of one person, the core of the application turned into a service, there were difficulties with the implementation.

Therefore, the initialization of the application through the CodeIgniter class is saved.

The Console class now uses the Services::commands() service directly instead of using the CommandRunner class.
The CodeIgniter class has been cleared of processing Spark commands, and the Console class no longer processes Response.

Although this is not within the purview of the PR, I added the StreamFilterTrait trait, which correctly adds and removes filters for capturing stream output, since the current implementation in the tests is not correct enough in my opinion.
Depends on StreamFilterTrait #6112

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-06-11T10:03:59Z

I have not looked into this PR yet, but this is a big change.
It would take some time to review.

So could you please split StreamFilterTrait into a separate PR?
It is small and will be merged sooner than this.

MGatner

I think this is the right direction. We have some big conversations going around CLI and the app so it is helpful to have an actual implementation to look at. Let's make this our general discussion place at well, just for the sake of having a common thread.

system/Test/StreamFilterTrait.php

iRedds · 2022-06-11T12:51:16Z

@kenjis StreamFilterTrait #6112

system/Config/Routes.php

kenjis · 2022-06-15T08:27:31Z

It seems there is no problem.

MGatner · 2022-06-15T10:31:15Z

I'm still in favor of this. The rest of the team is oddly quiet 😅 Let's get the User Guide and changelog updates done and then I will give them one last chance.

paulbalandan · 2022-06-16T02:35:57Z

system/CLI/Console.php


-        return $this->app->useSafeOutput($useSafeOutput)->run();
+        return $runner->run($command, $params);


BaseCommand::run() called by Commands::run() returns void although not strictly because not typehinted. However, most of our implementations of commands I think don't return anything either. So the $exit in L85 in spark above will be null so always exit on EXIT_SUCCESS?

It turns out that exit (0), unless of course the developer forcibly returns the desired value of the exit code.

paulbalandan · 2022-06-16T02:41:17Z

system/CodeIgniter.php

@@ -871,9 +847,7 @@ protected function createController()
     * CI4 supports three types of requests:
     *  1. Web: URI segments become parameters, sent to Controllers via Routes,
     *      output controlled by Headers to browser
-     *  2. Spark: accessed by CLI via the spark command, arguments are Command arguments,
-     *      sent to Commands by CommandRunner, output controlled by CLI class


Since this is removed, does this mean that the CommandRunner will be removed? Or deprecated, at least?

The CommandRunner class will be removed as there is no point in it. I'm already preparing changes.

tests/system/CLI/ConsoleTest.php

user_guide_src/source/incoming/routing.rst

user_guide_src/source/cli/cli_commands.rst

kenjis · 2022-06-17T01:33:44Z

@iRedds Thank you for updating the docs.

One last thing, Upgrading Guide also needed.
Like this: https://codeigniter4.github.io/CodeIgniter4/installation/upgrade_420.html#mandatory-file-changes

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#upgrading-guide

paulbalandan · 2022-06-17T02:34:07Z

user_guide_src/source/cli/cli_commands.rst

+===================
+
+By default, the command terminates with a success code of ``0``. To change the exit code, for example on error,
+the ``run()`` method must return an integer in the range 0 - 254. `PHP exit <https://www.php.net/manual/en/function.exit.php>`_.


Instead of specifying the range, I think it would be better to use our constants for that?

CodeIgniter4/app/Config/Constants.php

Lines 70 to 79 in b3ebf5b

defined('EXIT_SUCCESS') || define('EXIT_SUCCESS', 0); // no errors

defined('EXIT_ERROR') || define('EXIT_ERROR', 1); // generic error

defined('EXIT_CONFIG') || define('EXIT_CONFIG', 3); // configuration error

defined('EXIT_UNKNOWN_FILE') || define('EXIT_UNKNOWN_FILE', 4); // file not found

defined('EXIT_UNKNOWN_CLASS') || define('EXIT_UNKNOWN_CLASS', 5); // unknown class

defined('EXIT_UNKNOWN_METHOD') || define('EXIT_UNKNOWN_METHOD', 6); // unknown class member

defined('EXIT_USER_INPUT') || define('EXIT_USER_INPUT', 7); // invalid user input

defined('EXIT_DATABASE') || define('EXIT_DATABASE', 8); // database error

defined('EXIT__AUTO_MIN') || define('EXIT__AUTO_MIN', 9); // lowest automatically-assigned error code

defined('EXIT__AUTO_MAX') || define('EXIT__AUTO_MAX', 125); // highest automatically-assigned error code

Good suggestion! I forgot the constants.

Made changes. I hope this is similar to what you were talking about.

iRedds · 2022-06-17T11:15:38Z

@kenjis I have doubts about the quality of the upgrade manual.

user_guide_src/source/cli/cli_commands.rst

kenjis · 2022-06-17T23:31:28Z

Please add.

--- a/user_guide_src/source/installation/upgrading.rst
+++ b/user_guide_src/source/installation/upgrading.rst
@@ -12,6 +12,7 @@ upgrading from.
 .. toctree::
     :titlesonly:
 
+    upgrade_430
     upgrade_421
     upgrade_420
     upgrade_418

I think the upgrading is okay. Thanks.

kenjis

LGTM!

kenjis added 4.3 refactor Pull requests that refactor code labels Jun 11, 2022

MGatner reviewed Jun 11, 2022

View reviewed changes

system/Test/StreamFilterTrait.php Outdated Show resolved Hide resolved

iRedds requested a review from paulbalandan June 11, 2022 15:26

iRedds force-pushed the spark-rework branch from e2280cc to 1ab6243 Compare June 15, 2022 07:56

kenjis reviewed Jun 15, 2022

View reviewed changes

system/Config/Routes.php Outdated Show resolved Hide resolved

kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jun 15, 2022

paulbalandan reviewed Jun 16, 2022

View reviewed changes

iRedds added 2 commits June 17, 2022 00:41

Extracting the call handler for Spark commands from application.

775fa7d

spark-rework: docs&conf.

be9b054

iRedds force-pushed the spark-rework branch from c636299 to be9b054 Compare June 16, 2022 16:43

spark-rework: exit code

ef4e60f

kenjis reviewed Jun 17, 2022

View reviewed changes

user_guide_src/source/incoming/routing.rst Outdated Show resolved Hide resolved

kenjis reviewed Jun 17, 2022

View reviewed changes

user_guide_src/source/cli/cli_commands.rst Outdated Show resolved Hide resolved

paulbalandan reviewed Jun 17, 2022

View reviewed changes

spark-rework: upgrade_430.rst & other docs

0dc8b11

kenjis reviewed Jun 17, 2022

View reviewed changes

user_guide_src/source/cli/cli_commands.rst Outdated Show resolved Hide resolved

kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Jun 17, 2022

spark-rework: docs

1502105

MGatner approved these changes Jun 18, 2022

View reviewed changes

paulbalandan approved these changes Jun 18, 2022

View reviewed changes

kenjis approved these changes Jun 18, 2022

View reviewed changes

kenjis merged commit 6e5b412 into codeigniter4:4.3 Jun 18, 2022

iRedds deleted the spark-rework branch June 18, 2022 22:37

kenjis mentioned this pull request Jul 16, 2022

fix: spark routes does not show routes at all #6270

Merged

5 tasks

kenjis mentioned this pull request Jan 22, 2024

Spark no longer trigger pre_system and post_system events #8442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracting the call handler for Spark commands from kernel. #6110

Extracting the call handler for Spark commands from kernel. #6110

iRedds commented Jun 11, 2022 •

edited

Loading

kenjis commented Jun 11, 2022

MGatner left a comment

iRedds commented Jun 11, 2022

kenjis commented Jun 15, 2022

MGatner commented Jun 15, 2022

paulbalandan Jun 16, 2022

iRedds Jun 16, 2022

paulbalandan Jun 16, 2022

iRedds Jun 16, 2022

kenjis commented Jun 17, 2022

paulbalandan Jun 17, 2022

kenjis Jun 17, 2022

iRedds Jun 17, 2022

iRedds commented Jun 17, 2022

kenjis commented Jun 17, 2022

kenjis left a comment


		return $this->app->useSafeOutput($useSafeOutput)->run();
		return $runner->run($command, $params);

	defined('EXIT_SUCCESS') \|\| define('EXIT_SUCCESS', 0); // no errors
	defined('EXIT_ERROR') \|\| define('EXIT_ERROR', 1); // generic error
	defined('EXIT_CONFIG') \|\| define('EXIT_CONFIG', 3); // configuration error
	defined('EXIT_UNKNOWN_FILE') \|\| define('EXIT_UNKNOWN_FILE', 4); // file not found
	defined('EXIT_UNKNOWN_CLASS') \|\| define('EXIT_UNKNOWN_CLASS', 5); // unknown class
	defined('EXIT_UNKNOWN_METHOD') \|\| define('EXIT_UNKNOWN_METHOD', 6); // unknown class member
	defined('EXIT_USER_INPUT') \|\| define('EXIT_USER_INPUT', 7); // invalid user input
	defined('EXIT_DATABASE') \|\| define('EXIT_DATABASE', 8); // database error
	defined('EXIT__AUTO_MIN') \|\| define('EXIT__AUTO_MIN', 9); // lowest automatically-assigned error code
	defined('EXIT__AUTO_MAX') \|\| define('EXIT__AUTO_MAX', 125); // highest automatically-assigned error code

Extracting the call handler for Spark commands from kernel. #6110

Extracting the call handler for Spark commands from kernel. #6110

Conversation

iRedds commented Jun 11, 2022 • edited Loading

kenjis commented Jun 11, 2022

MGatner left a comment

Choose a reason for hiding this comment

iRedds commented Jun 11, 2022

kenjis commented Jun 15, 2022

MGatner commented Jun 15, 2022

paulbalandan Jun 16, 2022

Choose a reason for hiding this comment

iRedds Jun 16, 2022

Choose a reason for hiding this comment

paulbalandan Jun 16, 2022

Choose a reason for hiding this comment

iRedds Jun 16, 2022

Choose a reason for hiding this comment

kenjis commented Jun 17, 2022

paulbalandan Jun 17, 2022

Choose a reason for hiding this comment

kenjis Jun 17, 2022

Choose a reason for hiding this comment

iRedds Jun 17, 2022

Choose a reason for hiding this comment

iRedds commented Jun 17, 2022

kenjis commented Jun 17, 2022

kenjis left a comment

Choose a reason for hiding this comment

iRedds commented Jun 11, 2022 •

edited

Loading