Adding PHP 8.2 support

[gxgpet]

This adds support for PHP 8.2.

Basically, fixing dynamic props assigning.

[Khuthaily]

Thank you, @gxgpet. In addition to the changes your are proposing, I had to add #[AllowDynamicProperties] in system > core > Controller.php and system > core > Router.php in one of my projects to provide support for PHP 8.2.

[gxgpet]

Thanks for your suggestion, @Khuthaily! I added the attribute only for CI_Controller; as for the router, the uri property was missing only, and I added it. I guess you didn't find something different for the router, right?

[sneakyimp]

Where do we stand on 8.2 compatibility? Issue #6192 seems to basically be a duplicate of this issue and I'm pretty eager to apply this fix to a production system I'm upgrading to PHP 8.2. Are we sure it covers all the dynamic class assignments? Are there any unit tests dedicated to checking this? Has anyone used a static analysis tool to check for undeclared properties? I've checked out PHPStan expressly to find issues like this and it apparently doesn't detect undefined property assignments on objects instantiated via a factory method.

[sneakyimp]

In the interest of furthering the effort for 8.2 compatibility, I have downloaded the latest CI 3.1.13 and run php stan on it:

vendor/bin/phpstan analyse -l 2 codeigniter > phpstan-output.txt

This yields about 2090 errors (see attached phpstan-output.tgz), including 688 'Access to an undefined property' errors. One example of which is:

Access to an undefined property CI_Migration::$lang.

I grepped those lines and wrote a quick PHP script to list all the distinct classes before the '::' and this is the result:

$this(CI_DB_cubrid_driver)
$this(CI_DB_mssql_driver)
$this(CI_DB_mysql_driver)
$this(CI_DB_mysqli_driver)
$this(CI_DB_oci8_driver)
$this(CI_DB_pdo_cubrid_driver)
$this(CI_DB_pdo_dblib_driver)
$this(CI_DB_pdo_ibm_driver)
$this(CI_DB_pdo_mysql_driver)
$this(CI_DB_pdo_odbc_driver)
$this(CI_DB_pdo_sqlsrv_driver)
$this(CI_DB_postgre_driver)
$this(CI_DB_sqlsrv_driver)
CI_Controller
CI_DB_cubrid_driver
CI_DB_driver
CI_DB_ibase_driver
CI_DB_ibase_forge
CI_DB_mssql_driver
CI_DB_mysql_driver
CI_DB_mysqli_driver
CI_DB_oci8_driver
CI_DB_pdo_4d_driver
CI_DB_pdo_cubrid_driver
CI_DB_pdo_dblib_driver
CI_DB_pdo_driver
CI_DB_pdo_firebird_driver
CI_DB_pdo_firebird_forge
CI_DB_pdo_ibm_driver
CI_DB_pdo_informix_driver
CI_DB_pdo_mysql_driver
CI_DB_pdo_oci_driver
CI_DB_pdo_odbc_driver
CI_DB_pdo_pgsql_driver
CI_DB_pdo_pgsql_forge
CI_DB_pdo_result
CI_DB_pdo_sqlite_driver
CI_DB_pdo_sqlsrv_driver
CI_DB_postgre_driver
CI_DB_postgre_forge
CI_DB_sqlite3_driver
CI_DB_sqlite_driver
CI_DB_sqlsrv_driver
CI_Image_lib
CI_Input
CI_Javascript
CI_Jquery
CI_Migration
CI_Router
CI_Table
CI_URI
object

I am not at all certain, but it looks like we might have a bit more work to do if we want to insure CI3 is really ready for PHP 8.2.

Anyone have thoughts on this?

EDIT: It looks like PHPStan doesn't see the CI_DB class definition because CI3 defines this class in a conditional block. I think we might be able to ignore all the CI_DB_* classes because they are either extending CI_DB_query_builder or CI_DB_driver -- surely we can put the attribute in those files abstract parent classes, right?

[sneakyimp]

Is there any progress on supporting PHP 8.2? I've seen a lot of people posting issues about it but they are all being closed except for this one, which seems to have the most up-to-date information and pull request. I badly need to update a server running a CI3 website and I'd very much like to have the site work with PHP 8.2 so I can avoid yet another later migration.

I don't see any declaration of AllowDynamicProperties in this pull request before the critical CI_DB declaration here. Nor do I see one applied to the CI_DB_query_builder abstract class here. I don't actually know if you can put AllowDynamicProperties in front of an abstract class, but it seems to me that we might fix most of those CI_DB_x_driver classes reported by phpstan if we were to address those two class declarations.

CI_Controller already has the dynamic properties attribute declared here in this pull request.

That would leave the following classes reported by phpstan: CI_DB_ibase_forge CI_DB_pdo_firebird_forge CI_DB_pdo_pgsql_forge CI_DB_pdo_result CI_DB_postgre_forge CI_Image_lib CI_Input CI_Javascript CI_Jquery CI_Migration CI_Router CI_Table CI_URI

Those forge and result classes could be handled, I think, by declaring AllowDynamicProperties for their parent classes.

Curiously, CI_Javascript and CI_Jquery classes do not exist in this pull request, so I'm not sure what to do about them.

The various other classes might also be solved with an AllowDynamicProperties declaration. Personally, I see no harm in simply adding these various declarations, and then it seems we are likely to have a viable PHP 8.2 fix, no?

Do we have (or need) Unit tests to trigger the depreciated warning for these classes? What must we do to make progress on this issue?

[sneakyimp]

It occurred to me to run phpstan on gxgpet's pull request instead of the CI3 codebase, and PHPstan is reporting a lot fewer errors. See attached gxgpet-output.zip which I generated like so:

vendor/bin/phpstan analyse -l 2 gxgpet > gxgpet-output.txt

This is still reporting about 1000 errors, but only 54 of those refer to undefined properties:

$ grep 'undefined prop' gxgpet-output.txt
  410    Access to an undefined property object::$directory.
  412    Access to an undefined property object::$directory.
  281    Access to an undefined property CI_Input::$raw_input_stream.
  465    Access to an undefined property object::$dbdriver.
  465    Access to an undefined property object::$dbdriver.
  469    Access to an undefined property object::$dbdriver.
  182    Access to an undefined property object::$dbdriver.
  182    Access to an undefined property object::$dbdriver.
  187    Access to an undefined property object::$dbdriver.
  1480   Access to an undefined property CI_DB_driver::$qb_limit.
  474    Access to an undefined property object::$dbprefix.
  545    Access to an undefined property object::$dbprefix.
  548    Access to an undefined property object::$dbprefix.
  102    Access to an undefined property CI_DB_ibase_forge::$hostname.
  117    Access to an undefined property CI_DB_ibase_forge::$conn_id.
  123    Access to an undefined property object::$database.
  157    Access to an undefined property CI_DB_mysql_driver::$db.
  230    Access to an undefined property CI_DB_mysqli_driver::$db.
  146    Access to an undefined property CI_DB_pdo_result::$db.
  148    Access to an undefined property CI_DB_pdo_result::$db.
  88     Access to an undefined property CI_DB_pdo_firebird_forge::$hostname.
  103    Access to an undefined property CI_DB_pdo_firebird_forge::$conn_id.
  109    Access to an undefined property object::$database.
  102    Access to an undefined property CI_DB_pdo_pgsql_forge::$create_table_if.
  133    Access to an undefined property object::$database.
  162    Access to an undefined property CI_DB_postgre_driver::$db.
  90     Access to an undefined property CI_DB_postgre_forge::$create_table_if.
  119    Access to an undefined property object::$database.
  300    Access to an undefined property CI_DB_sqlsrv_driver::$_escape_like_chr.
  300    Access to an undefined property CI_DB_sqlsrv_driver::$_escape_like_str.
  558    Access to an undefined property CI_Image_lib::$dest_image.
  563    Access to an undefined property CI_Image_lib::$dest_image.
  571    Access to an undefined property CI_Image_lib::$dest_image.
  577    Access to an undefined property CI_Image_lib::$dest_image.
  145    Access to an undefined property CI_Migration::$lang.
  148    Access to an undefined property CI_Migration::$load.
  168    Access to an undefined property CI_Migration::$db.
  170    Access to an undefined property CI_Migration::$dbforge.
  174    Access to an undefined property CI_Migration::$dbforge.
  176    Access to an undefined property CI_Migration::$db.
  215    Access to an undefined property CI_Migration::$lang.
  276    Access to an undefined property CI_Migration::$lang.
  289    Access to an undefined property CI_Migration::$lang.
  294    Access to an undefined property CI_Migration::$lang.
  337    Access to an undefined property CI_Migration::$lang.
  396    Access to an undefined property CI_Migration::$lang.
  446    Access to an undefined property CI_Migration::$db.
  460    Access to an undefined property CI_Migration::$db.
  521    Access to an undefined property object::$lang.
  521    Access to an undefined property object::$lang.
  521    Access to an undefined property object::$lang.
  521    Access to an undefined property object::$lang.
  521    Access to an undefined property object::$lang.
  43     Access to an undefined property CI_TestCase::$ci_view_root.

I have started picking through some of these and believe that quiet a few might be resolved by declaring var types with php doc comments. For example, those first two complaints are in system/core/CodeIgniter.php because we don't declare a var type for the var $RTR:

/*
 * ------------------------------------------------------
 *  Instantiate the routing class and set the routing
 * ------------------------------------------------------
 */
        $RTR =& load_class('Router', 'core', isset($routing) ? $routing : NULL);

Those first two PHPStan complaints could be resolved if we add an extra asterisk and @var declaration to the comment so it declares the var type:

/**
 * ------------------------------------------------------
 *  Instantiate the routing class and set the routing
 * ------------------------------------------------------
 * @var CI_Router|NULL
 */
        $RTR =& load_class('Router', 'core', isset($routing) ? $routing : NULL);

We might similarly remedy the complaints about object::$dbdriver in system/core/Loader.php by declaring a type more specific than simply 'object.'

Numerous complaints in Loader.php are due to the bad return type declaration in this function:

        /**
         * CI Component getter
         *
         * Get a reference to a specific library or model.
         *
         * @param       string  $component      Component name
         * @return      bool
         */
        protected function &_ci_get_component($component)
        {
                $CI =& get_instance();
                return $CI->$component;
        }

This peculiar function looks suspiciously similar to a __get magic method but you'd have to expressly invoke it rather than relying on 'magic' invocation. A grep search of the code shows that this function is only invoked in ./system/core/Loader.php to retrieve a config object. It is never used in any other file and never used to retrieve anything but a config object. We can rid ourselves of several PHPStan errors simply by changing the return declaration from @return bool to @return object.

I'd be happy to try and put in a little time to try and get rid of these 54 'undefined property' errors if folks think that would be a good idea. I would hate to go through the trouble only to have my efforts ignored.

Thoughts? Anyone?

[steveadamsfixedit]

I'd very much like to see PHP 8.2 supported...

[gxgpet]

@sneakyimp

Are we sure it covers all the dynamic class assignments?

No, we are not, that's why I waited for more reports to come up... As part of CI 3's architecture, many of those can be found only if the code is actually executed.

In the interest of furthering the effort for 8.2 compatibility, I have downloaded the latest CI 3.1.13 and run php stan on it:

This is indeed a very easy step to do, and I made it too with another tool; the problem is that the output of such a tool is containing a lot of false positives, and they must be sorted out... No such tool can understand properly how the code works, it's just static analysis.

But anyway, if anyone is concerned with the file you uploaded, here's the analysis I could made for it:

• We added #[AllowDynamicProperties] on CI_DB_driver, which is the root class for all implementations: CI_DB_query_builder extends CI_DB_driver, CI_DB extends CI_DB_query_builder. This can be easily found & proven if the code is opened with an IDE. So there's no issue on the DB layer and that only class which has the attribute is enough.
• We are not discussing Access to an undefined property X, Method X() should return Y but return statement is missing, Constant X not found and Variable X might not be defined.. These are obvious false positives, and they are not even related to the new PHP version.
• PHPDoc tag X has invalid value - nothing here
• CI_Migration has the __get magic method implemented
• CI_Image_lib - a commit was pushed with adding the missing property
• CI_Input has the __get magic method implemented.
• CI_Javascript, CI_Jquery - these were removed in 2016
• CI_Router, CI_Table and CI_URI are already changed here

Sorry, but any bug/issue we might have with PHP 8.2 must be reviewed and checked by a human eye to see if that's the case. We can't change the entire code just to avoid any false positives from a static analysis tool, we won't do it.

I'd very much like to see PHP 8.2 supported...

Well, I guess we are at the end of it since nothing more popped up recently. I think we shall deploy a new version soon.

[sneakyimp]

As I said last week:

I'd be happy to try and put in a little time to try and get rid of these 54 'undefined property' errors if folks think that would be a good idea. I would hate to go through the trouble only to have my efforts ignored.

I went and examined every 'undefined property' complaint from PHPStan and fixed several bugs and hundreds of bad PHPDoc declarations in #6198. That PR contains every change you've made in this one, and numerous other improvements as well. I believe we should use that PR instead. Either that or merge its changes into this PR. If you don't like the change to the file system/database/DB.php, you can leave it out. Personally, I think it's weird and bad style to dynamically declare that trivial CI_DB class inside a conditional, which is inside a function. I think the reason for that kludge in the legacy CI3 code is an inept way of supporting unit testing, which is also poorly implemented in this project.

[NielBuys]

Can somebody give me the "composer.json" changes/steps of how I can test this Pull request. I want to test and see if my app is compatible with this pull request.

[NelsonXavier30]

I couldn't figure out how to use my application without these errors

[NelsonXavier30]

someone to help me?

[NielBuys]

I created a patch file from this pull request and merged the patch file into my fork of CodeIgniter to test my app.

I had 2 more errors I needed to fix in CodeIgniter before my app started working. I needed to add "AllowDynamicProperties" to Controller.php and Router.php. See attached patch file for my changes.

NOTE: I see some changes was not included in original patch file. The below changes is not needed to be added. I changed my branch to include all commits of this pull request.

Add AllowDynamicProperties to Controller and Router.zip

I created a test environment for myself If somebody want to use it temporarily you are welcome. I can't guarantee the branch and I don't support it. Its only for myself.

What I needed to change in my app to use it.

1. Change "codeigniter/framework" to "nielbuys/framework" in the "composer.json" file.
2. Change in main "index.php" the following line from "$system_path = 'vendor/codeigniter/framework/system';" to "$system_path = 'vendor/nielbuys/framework/system';"
3. Then run composer update. Then composer will install the test framework v3.1.13.2

My app works in my test environment.

Here is the patch file I created from this pull request for record purposes. 6173.zip

Thanks all

[sneakyimp]

I had 2 more errors I needed to fix in CodeIgniter before my app started working. I needed to add "AllowDynamicProperties" to Controller.php and Router.php. See attached patch file for my changes.

I think you may a little confused about which code you are working with. The pull request discussed in this issue is the one suggested by gxgpet, and you can see in that pull request that he has the AllowDynamicProperties declaration in Controller and in Router he added a property or two to hopefully resolve dynamic property complaints.

[wagnerfnds]

Honestly it is sad to see codeigniter3 dying, just now that PHP is comin to be a very modern language. Time to migrate our projects i think.

[RedDragonWebDesign]

What are next steps to get a code review and merge for this? Looks like it's been sitting for 4 months. Do we need to ping some additional collaborators? @gxgpet looks like you have merge access for this repo, any additional thoughts?

[RedDragonWebDesign]

@narfbg

[NielBuys]

I had 2 more errors I needed to fix in CodeIgniter before my app started working. I needed to add "AllowDynamicProperties" to Controller.php and Router.php. See attached patch file for my changes.

I think you may a little confused about which code you are working with. The pull request discussed in this issue is the one suggested by gxgpet, and you can see in that pull request that he has the AllowDynamicProperties declaration in Controller and in Router he added a property or two to hopefully resolve dynamic property complaints.

Thanks for letting me know. I have realized after I created the post, that the patch file from Github did not include those 2 changes. I added a note to my original post that I have changed my branch to exactly match what is in this pull request.

[colinalbion]

We have substantial CI3 applications running nicely on php 7.4 that we don't want to rewrite for CI4 just to be PHP 8x and 9.x compatible. We need a version of CI3 that is compatible with 7.4 and above (ideally php8.1 and eventually php9). I'd happily support an initiative to do this.

To rewrite for CI4 would involve upheaval to 20k+ lines of code whereas making CI 3 compatible with PHP 8+ would perhaps be just a few hundred of lines of code of change.

Anyone else in the same position?

Edit: After a few hours....added upgraded to 3.1.13, then added #[AllowDynamicProperties] to a few system files (Controler, URI etc) and a few of our own libs, we now have our app working with PHP8.2 (very pleased). I recognise that this is not the final solution, but it is reassuring that there is a path forward where we don't have to entirely rewrite tens of thousands of CI3 code into CI4 or Laraval. Again, it would be great to see a CI3 version that is compatible with PHP 8+ (perhaps dropping php5 support).

[codestellar]

Will this branch be merged? I am looking forward to see php 8.2 support for CI3.

[mohammadhzp]

@narfbg Are you able to merge this pull request?

[nillander]

Eu tentei atualizar o meu CI para trabalhar com o PHP8.2 mas não consegui, o melhor que consegui foi o PHP8.1

[nillander]

We have substantial CI3 applications running nicely on php 7.4 that we don't want to rewrite for CI4 just to be PHP 8x and 9.x compatible. We need a version of CI3 that is compatible with 7.4 and above (ideally php8.1 and eventually php9). I'd happily support an initiative to do this.

To rewrite for CI4 would involve upheaval to 20k+ lines of code whereas making CI 3 compatible with PHP 8+ would perhaps be just a few hundred of lines of code of change.

Anyone else in the same position?

Edit: After a few hours....added upgraded to 3.1.13, then added #[AllowDynamicProperties] to a few system files (Controler, URI etc) and a few of our own libs, we now have our app working with PHP8.2 (very pleased). I recognise that this is not the final solution, but it is reassuring that there is a path forward where we don't have to entirely rewrite tens of thousands of CI3 code into CI4 or Laraval. Again, it would be great to see a CI3 version that is compatible with PHP 8+ (perhaps dropping php5 support).

Amigo, você poderia dizer como fez para compatibilizar o CI3 com o PHP8.2?

[thaibmt]

Eu tentei atualizar o meu CI para trabalhar com o PHP8.2 mas não consegui, o melhor que consegui foi o PHP8.1

I can support you make CI support php 8.2. please contact me

[nillander]

Eu tentei atualizar o meu CI para trabalhar com o PHP8.2 mas não consegui, o melhor que consegui foi o PHP8.1

I can support you make CI support php 8.2. please contact me

Did I contact you by email

[thaibmt]

Yes. You can contact me by my email

[nillander]

<removed>

[totoprayogo1916]

@NielBuys Can somebody give me the "composer.json" changes/steps of how I can test this Pull request. I want to test and see if my app is compatible with this pull request.

    "require": {

        "codeigniter/framework": "dev-develop"

     },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/gxgpet/CodeIgniter.git"
        }
    ]

[madwings]

Is anyone from the team interested in merging this pull request?

[tenzap]

Another item that was probably missed:

An uncaught Exception was encountered

Type: ErrorException

Message: Creation of dynamic property CI_DB_postgre_forge::$create_table_if is deprecated

Filename: system/database/drivers/postgre/postgre_forge.php

Line Number: 90