refactor: CodeIgniter has context #5650

kenjis · 2022-02-03T08:36:11Z

Description
Supersede: #5649

add CodeIgniter::$context: web, php-cli, spark
make the constant SPARKED deprecated
remove is_cli() in CodeIgniter

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-02-03T11:25:59Z

@samsonasik Is this a bug of rector?

1 file with changes
===================

1) system/CodeIgniter.php:369

    ---------- begin diff ----------
@@ @@
     }

     /**
-     * Invoked via php-cli command?
-     */
-    private function isPhpCli(): bool
-    {
-        return $this->context === 'php-cli';
-    }
-
-    /**
      * Web access?
      */
     private function isWeb(): bool
    ----------- end diff -----------

Applied rules:
 * RemoveUnusedPrivateMethodRector

https://github.com/codeigniter4/CodeIgniter4/runs/5049699602?check_suite_focus=true

samsonasik · 2022-02-03T18:01:38Z

Possibly, could you create failing test case? You can start with getrector.org/demo and click "Create a Test" after submit reproduced bug

kenjis · 2022-02-04T00:57:19Z

@samsonasik

could you create failing test case?

No, I tried on getrector.org/demo, but RemoveUnusedPrivateMethodRector was not applied.

MGatner

I like this much better than your first attempt. Some comments. Also, I think we should set a default context on MockCodeIgniter (probably "web"?) so people's tests don't start failing unexpectedly.

spark

system/CodeIgniter.php

kenjis · 2022-02-06T02:28:32Z

Also, I think we should set a default context on MockCodeIgniter (probably "web"?) so people's tests don't start failing unexpectedly.

Added.

system/CodeIgniter.php

kenjis · 2022-02-06T10:13:31Z

@MGatner

Unless there is a compelling reason for these methods I prefer the simple property check inline.

The simple property check inline is too complex to me, as @samsonasik pointed out.
Can I use private methods?

MGatner

Looking good, still a few conversations going.

MGatner · 2022-02-06T13:13:42Z

system/Test/Mock/MockCodeIgniter.php

+    /**
+     * Context
+     *  web:     Invoked by HTTP request
+     *  php-cli: Invoked by CLI via `php public/index.php`
+     *  spark:   Invoked by CLI via the `spark` command
+     *
+     * @phpstan-var 'php-cli'|'spark'|'web'
+     */


IDEs will use the parent docblock so it's fine to drop this.

tests/system/ControllerTest.php

MGatner · 2022-02-06T13:15:28Z

tests/system/RESTful/ResourceControllerTest.php

@@ -65,6 +65,7 @@ protected function setUp(): void

        $config            = new App();
        $this->codeigniter = new MockCodeIgniter($config);
+        $this->codeigniter->setContext('web');


I think these are no longer needed with the default property?

MGatner · 2022-02-06T13:15:37Z

tests/system/RESTful/ResourcePresenterTest.php

@@ -59,6 +59,7 @@ protected function setUp(): void

        $config            = new App();
        $this->codeigniter = new MockCodeIgniter($config);
+        $this->codeigniter->setContext('web');


I think these are no longer needed with the default property?

MGatner · 2022-02-06T13:22:53Z

The simple property check inline is too complex to me, as @samsonasik pointed out.

Can I use private methods?

I'm fine with the private methods. It still isn't my preference but I don't think either is "wrong" and I trust you as the author, unless there are other stronger opinions.

system/CodeIgniter.php

Make the constant SPARKED deprecated. Remove is_cli() in CodeIgniter.

It is MockCodeIgniter's default.

kenjis · 2022-02-16T23:11:27Z

@MGatner Can you review?

MGatner

Looks good. Should spark and public/index.php be included in the "All Changes" section?

kenjis · 2022-02-18T23:34:40Z

Should spark and public/index.php be included in the "All Changes" section?

Strictly speaking no, I think. Because:

All changes noted here are optional until the next major version,
and any mandatory changes will be covered in the sections above.

samsonasik · 2022-02-18T23:46:02Z

@kenjis just tried a development build in a ci4 project:

php builds development
composer update
php spark serve

CodeIgniter v4.1.8 Command Line Tool - Server Time: 2022-02-18 17:42:23 UTC-06:00

An uncaught Exception was encountered

Type:        Error
Message:     Typed property CodeIgniter\CodeIgniter::$context must not be accessed before initialization
Filename:    /Users/samsonasik/www/ci4-prj/vendor/codeigniter4/codeigniter4/system/CodeIgniter.php
Line Number: 308

        Backtrace:
                                                -48 - /Users/samsonasik/www/ci4-prj/vendor/codeigniter4/codeigniter4/system/CLI/Console.php::run
                                                                -57 - /Users/samsonasik/www/ci4-prj/spark::run

It seems the warning is not shown early for :

Context must be set before run() is called. If you are upgrading from 4.1.x, you need to merge `public/index.php` and `spark` file from `vendor/codeigniter4/framework`.

as it not initialized.

kenjis · 2022-02-18T23:52:10Z

@samsonasik
Can't reproduce.

$ php spark serve

CodeIgniter v4.1.8 Command Line Tool - Server Time: 2022-02-18 17:51:33 UTC-06:00

CodeIgniter development server started on http://localhost:8080
Press Control-C to stop.
[Sat Feb 19 08:51:33 2022] PHP 8.0.15 Development Server (http://localhost:8080) started

samsonasik · 2022-02-18T23:55:21Z

@kenjis I created PR #5708 for it, the spark and public/index.php not yet updated, so the $context is accessed early, tested in php 8.1

kenjis · 2022-02-18T23:56:33Z

@samsonasik I got it!

kenjis added the refactor Pull requests that refactor code label Feb 3, 2022

kenjis force-pushed the remove-SPARKED branch from ef473c6 to 8d76a03 Compare February 3, 2022 09:55

MGatner requested changes Feb 5, 2022

View reviewed changes

spark Show resolved Hide resolved

system/CodeIgniter.php Show resolved Hide resolved

system/CodeIgniter.php Outdated Show resolved Hide resolved

system/CodeIgniter.php Show resolved Hide resolved

samsonasik reviewed Feb 6, 2022

View reviewed changes

system/CodeIgniter.php Outdated Show resolved Hide resolved

samsonasik reviewed Feb 6, 2022

View reviewed changes

system/CodeIgniter.php Outdated Show resolved Hide resolved

MGatner requested changes Feb 6, 2022

View reviewed changes

kenjis force-pushed the remove-SPARKED branch from 3cf1c50 to 822adfd Compare February 8, 2022 01:16

kenjis requested review from samsonasik and MGatner February 8, 2022 01:55

samsonasik approved these changes Feb 8, 2022

View reviewed changes

samsonasik reviewed Feb 8, 2022

View reviewed changes

system/CodeIgniter.php Outdated Show resolved Hide resolved

kenjis requested a review from samsonasik February 10, 2022 04:14

kenjis added 9 commits February 12, 2022 17:08

refactor: CodeIgniter has context: web, php-cli, spark

8815745

Make the constant SPARKED deprecated. Remove is_cli() in CodeIgniter.

docs: improve PHPDocs

641aa56

test: add default $context = 'web' in MockCodeIgniter

ce41eef

docs: drop PHPDocs

0f424a8

test: remove setContext('web')

2f9f580

It is MockCodeIgniter's default.

style: add parentheses to avoid confusion

9b58580

chore: RemoveUnusedPrivateMethodRector skips CodeIgniter.php

6eb8bf7

docs: add upgrade note

c6ee79f

refactor: improve error message

a10bde7

kenjis force-pushed the remove-SPARKED branch from 729ba95 to a10bde7 Compare February 12, 2022 08:12

samsonasik approved these changes Feb 16, 2022

View reviewed changes

MGatner approved these changes Feb 18, 2022

View reviewed changes

kenjis merged commit bc96617 into codeigniter4:develop Feb 18, 2022

kenjis deleted the remove-SPARKED branch February 18, 2022 23:36

samsonasik mentioned this pull request Feb 18, 2022

Make CodeIgniter::context nullable to handle access before initialization #5708

Merged

1 task

kenjis mentioned this pull request Feb 21, 2022

docs: update changelogs/v4.2.0.rst #5716

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: CodeIgniter has context #5650

refactor: CodeIgniter has context #5650

kenjis commented Feb 3, 2022 •

edited

Loading

kenjis commented Feb 3, 2022 •

edited

Loading

samsonasik commented Feb 3, 2022

kenjis commented Feb 4, 2022

MGatner left a comment

kenjis commented Feb 6, 2022

kenjis commented Feb 6, 2022

MGatner left a comment

MGatner Feb 6, 2022

kenjis Feb 8, 2022

MGatner Feb 6, 2022

kenjis Feb 8, 2022

MGatner Feb 6, 2022

kenjis Feb 8, 2022

MGatner commented Feb 6, 2022

kenjis commented Feb 16, 2022

MGatner left a comment

kenjis commented Feb 18, 2022

samsonasik commented Feb 18, 2022

kenjis commented Feb 18, 2022

samsonasik commented Feb 18, 2022

kenjis commented Feb 18, 2022

refactor: CodeIgniter has context #5650

refactor: CodeIgniter has context #5650

Conversation

kenjis commented Feb 3, 2022 • edited Loading

kenjis commented Feb 3, 2022 • edited Loading

samsonasik commented Feb 3, 2022

kenjis commented Feb 4, 2022

MGatner left a comment

Choose a reason for hiding this comment

kenjis commented Feb 6, 2022

kenjis commented Feb 6, 2022

MGatner left a comment

Choose a reason for hiding this comment

MGatner Feb 6, 2022

Choose a reason for hiding this comment

kenjis Feb 8, 2022

Choose a reason for hiding this comment

MGatner Feb 6, 2022

Choose a reason for hiding this comment

kenjis Feb 8, 2022

Choose a reason for hiding this comment

MGatner Feb 6, 2022

Choose a reason for hiding this comment

kenjis Feb 8, 2022

Choose a reason for hiding this comment

MGatner commented Feb 6, 2022

kenjis commented Feb 16, 2022

MGatner left a comment

Choose a reason for hiding this comment

kenjis commented Feb 18, 2022

samsonasik commented Feb 18, 2022

kenjis commented Feb 18, 2022

samsonasik commented Feb 18, 2022

kenjis commented Feb 18, 2022

kenjis commented Feb 3, 2022 •

edited

Loading

kenjis commented Feb 3, 2022 •

edited

Loading