Adding PHP 8.2 support - Githubissues

gxgpet commented 2 years ago

This adds support for PHP 8.2.

Basically, fixing dynamic props assigning.

Khuthaily commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 CIDB* 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gxgpet commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

NelsonXavier30 commented 1 year ago

someone to help me?

NielBuys commented 1 year ago

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.

Change "codeigniter/framework" to "nielbuys/framework" in the "composer.json" file.
Change in main "index.php" the following line from "$system_path = 'vendor/codeigniter/framework/system';" to "$system_path = 'vendor/nielbuys/framework/system';"
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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@narfbg

NielBuys commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mohammadhzp commented 1 year ago

@narfbg Are you able to merge this pull request?

nillander commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Yes. You can contact me by my email

nillander commented 1 year ago

<removed>

totoprayogo1916 commented 1 year ago

@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 commented 1 year ago

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

tenzap commented 1 year ago

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

carinihdev commented 1 year ago

Can this branch be merged?

frost-byte commented 1 year ago

What's the status of this? Is it going to be merged?

fernandoribeiro7 commented 1 year ago

What is the expected release date for the merge?

mmihalev commented 1 year ago

Any idea when this is going to be merged? Having support for php 8.2 is a must!

madwings commented 1 year ago

Any idea when this is going to be merged? Having support for php 8.2 is a must!

Most likely never or not in the near future. This project is dead. BCIT and CodeIgniter Foundation have focused on CI4 and CI3 is left to dust. Sad but this is the reality, they are not even releasing the project from their hands so it is transferred to someone else and continues the support. If you want PHP8.2 support - fork it and do it by yourself. This is the way now.

mmihalev commented 1 year ago

@narfbg , Нарфе, ела го merge-ни бре...

pocketarc commented 1 year ago

Honestly, that's what I did - I forked it, and been using it ever since. I'll maintain it for 8.3 as well. I'm not interested in dropping compatibility with anything or making any big changes, it's literally just to keep old projects running on an up-to-date system.

Edit: Feel free to use the fork I maintain, it's easy to set it up in composer.json as a custom repository, so it replaces the original codeigniter package. If you didn't install CI via composer.json, just grab the updated files as you normally do.

mohammadhzp commented 1 year ago

Any idea when this is going to be merged? Having support for php 8.2 is a must!

Most likely never or not in the near future. This project is dead. BCIT and CodeIgniter Foundation have focused on CI4 and CI3 is left to dust. Sad but this is the reality, they are not even releasing the project from their hands so it is transferred to someone else and continues the support. If you want PHP8.2 support - fork it and do it by yourself. This is the way now.

As far as I can tell, It doesn't look dead.

PHP 8.2 was officially released on December 8, 2022, this PR opened a month before the final stable release.
CI3 is continuing what CI2 started, which means many projects built on CI2 over the years easily upgraded to CI3, but that's not the case with CI4. It's new, and new projects are using it only. That's an important point.
CI3 is super stable, it's fast. even its security module is ported(Anti-XSS) and is popular.

RedDragonWebDesign commented 1 year ago

@pocketarc - can you publish a release (https://github.com/pocketarc/codeigniter/releases) so that https://github.com/pocketarc/codeigniter is easier for me to install in composer? maybe also turn on the issues tab so I can file a ticket for this kind of thing in the future? thanks

kenjis commented 1 year ago

Forking a project is generally a bad option. Many forked projects have failed in the past.

If you are willing to maintain the project, it seems to me that you should first find a way to become a maintainer. See

pocketarc commented 1 year ago

@RedDragonWebDesign Package is up; pocketarc/codeigniter. Issues are up, too; happy to discuss anything there.

@kenjis It'd be great to become a maintainer, but I don't have any existing contributions to CI's repo, and so understandably they'd have no reason to trust me as a maintainer. It's also not something I particularly care about. All I want is to make sure some old CI projects I'm involved with can keep running on up-to-date servers without problems.

ewwink commented 1 year ago

I'm using XAMPP (windows), is anyone experiencing high CPU usage using #[AllowDynamicProperties]?

jamieburchell commented 10 months ago

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.

@gxgpet Please can you merge this and release a new version?

rickhewes commented 10 months ago

Would be great to see this merged @gxgpet !

:+1:

tenzap commented 10 months ago

ubuntu-24.04-feature-freeze which is the next LTS Ubuntu will be on 2024-02-29.

The changes of this PR were added to 3.1.13 package in debian/ubuntu package so that it works, but if you could release a new version working up to latest php version soon that would be nice and the package could be updated accordingly. But 4 weeks is short.

jamieburchell commented 10 months ago

At this point the only option is to make your own fork and apply your own patches.