PHP 8.2 compatibility, a couple of bug fixes, move CI_DB class declaration

[sneakyimp]

This pull request includes all of the changes in pull request Adding PHP 8.2 support by @gxgpet, but applied to the latest develop branch of CI3 as of this writing, a6faab. I think one of gxgpet's commits has my name attached to it because git was complaining about commits with a private email address.

I have also made changes to reduce the number of complaints by PHPStan from about 1830 to about 560. Anyone who wants to check the PHPStan scan of this PR can do so by running this from the command line on a machine with PHP 8.2.3. It works for me on both Ubuntu and MacOS. NOTE that this phpstan config excludes the tests and user_guide_src from analysis.

# make a dir
mkdir ci3-pr
cd ci3-pr
# fetch sneakyimp PR source
curl -o ci.zip https://codeload.github.com/sneakyimp/CodeIgniter/zip/refs/heads/develop
unzip ci.zip
mv CodeIgniter-develop codeigniter
rm ci.zip
# install composer
php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
php -r "if (hash_file('sha384', 'composer-setup.php') === '55ce33d7678c5a611085589f1f3ddf8b3c52d662cd01d4ba75c0ee0459970c2200a51f492d557530c71c15d8dba01eae') { echo 'Installer verified'; } else { echo 'Installer corrupt'; unlink('composer-setup.php'); } echo PHP_EOL;"
php composer-setup.php
php -r "unlink('composer-setup.php');"
# user composer to install phpstan
./composer.phar require --dev phpstan/phpstan
# create a ci.neon config file for phpstan
echo -e "parameters:" >> ci.neon
echo -e "\tlevel: 2" >> ci.neon
echo -e "\tpaths:" >> ci.neon
echo -e "\t\t- codeigniter" >> ci.neon
echo -e "\texcludePaths:" >> ci.neon
echo -e "\t\tanalyse:" >> ci.neon
echo -e "\t\t\t- codeigniter/tests" >> ci.neon
echo -e "\t\t\t- codeigniter/user_guide_src" >> ci.neon
# clear the phpstan cache, if any
vendor/bin/phpstan clear-result-cache -c ci.neon
# run the phpstan analysis
vendor/bin/phpstan analyse -c ci.neon > phpstan.txt

This downloads my fork into a subdir, ci3-pr, and runs phpstan on it via composer. You can inspect the file ci3-pr/phpstan.txt for the results:

tail ci3-pr/phpstan.txt

The most significant source code hange is worth describing specifically. CI3 has awkwardly declared the CI_DB class inside a conditional that is inside the function DB() defined in system/database/DB.php:

    require_once(BASEPATH.'database/DB_driver.php');
    require_once(BASEPATH.'database/DB_query_builder.php');
    if ( ! class_exists('CI_DB', FALSE))
    {
        /**
         * CI_DB
         *
         * Acts as an alias for both CI_DB_driver and CI_DB_query_builder.
         *
         * @see CI_DB_query_builder
         * @see CI_DB_driver
         */
        class CI_DB extends CI_DB_query_builder {}
    }

I can see no reason for this class to be declared in this strange way. Examining the code base, the file system/database/DB.php will only ever be included/required once in system/core/Loader.php or tests/mocks/database/db.php so it's entirely safe to put those require_once statements and the CI_DB class declaration at the beginning of the file. It will not be included/required twice, and the DB() function will be more efficient without that conditional being evaluated every time. This also eliminates a vast number of phpstan complaints as described above.

Aside from this possibly significant change, the vast majority of changes I have made are simply to fix bad phpdoc parameter declarations. I have also changed a couple of phpdoc @var declarations. There are one or two changes to actual variables described below.

To further the goal of PHP 8.2 compatibility, I ran phpstan on my code and looked for all the ‘undefined property’ errors. I think they have all been addressed. See below for explanations as to why I think we have handled the various remaining errors.

system/core/Input.php 281 Access to an undefined property CI_Input::$raw_input_stream. This class defines a protected $_raw_input_stream and also a __get method. The intent seems to be to set this value at the last possible moment or something. I don’t think it’ll cause a problem but not sure how to get rid of the phpstan error.

system/core/Loader.php 403 Access to an undefined property CI_Controller::$db. 438 Access to an undefined property CI_Controller::$dbutil 465 Access to an undefined property object::$dbdriver. 465 Access to an undefined property object::$dbdriver. 469 Access to an undefined property object::$dbdriver. 482 Access to an undefined property CI_Controller::$dbforge. 694 Access to an undefined property CI_Controller::$lang. 713 Access to an undefined property CI_Controller::$config. 1016 Access to an undefined property CI_Controller::$output. The CI_Controller class declaration is preceded by the #[AllowDynamicProperties] attribute so these should be fine. NOTE that most of these complaints could probably be eliminated by simply declaring public $db in CI_Controller as a suitable CI_DB type.

system/core/Output.php 523 Access to an undefined property CI_Controller::$profiler. 528 Access to an undefined property CI_Controller::$profiler. 561 Access to an undefined property CI_Controller::$config. 570 Access to an undefined property CI_Controller::$config. 571 Access to an undefined property CI_Controller::$config. 572 Access to an undefined property CI_Controller::$uri. 574 Access to an undefined property CI_Controller::$config. 740 Access to an undefined property CI_Controller::$config. 754 Access to an undefined property CI_Controller::$uri. 756 Access to an undefined property CI_Controller::$config. 769 Access to an undefined property CI_Controller::$config. 769 Access to an undefined property CI_Controller::$config. The CI_Controller class declaration is preceded by the #[AllowDynamicProperties] attribute so these should be fine. We could probably eliminate a lot more phpstan errors if we simply added $config & $lang properties to the CI_Controller class.

system/database/DB_driver.php 1480 Access to an undefined property CI_DB_driver::$qb_limit. The abstract class CI_DB_driver is preceded by the #[AllowDynamicProperties] attribute so this should be fine

system/database/DB_forge.php 474 Access to an undefined property object::$dbprefix. 545 Access to an undefined property object::$dbprefix. 548 Access to an undefined property object::$dbprefix. OK these complaints are weird because other lines, including line 464 in the same function, also refers to $this->db->dbprefix without complaint. The DB object should probably have a dbprefix so perhaps we don’t need to worry about this. 🤷

system/database/DB_utility.php 400 Access to an undefined property CI_Controller::$zip. 401 Access to an undefined property CI_Controller::$zip. The CI_Controller class declaration is preceded by the #[AllowDynamicProperties] attribute so these should be fine. NOTE: I see some type hinting parameter declarations in the functions csv_from_result and xml_from_result — this might be incompatible with PHP 5? I suggest we remove these.

system/database/drivers/ibase/ibase_forge.php 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. I don’t know what to do about these. Other forge objects don’t seem to refer to hostname or conn_id. The $database is referenced on $this->db->database. I punt this to someone who know what ibase is.

system/database/drivers/mysql/mysql_driver.php 157 Access to an undefined property CI_DB_mysql_driver::$db. This looks like it was actually a bug, referring to $this->db->debug when it should have referred to $this->db_debug.

system/database/drivers/mysqli/mysqli_driver.php 230 Access to an undefined property CI_DB_mysqli_driver::$db. Another bug, referring to $this->db->db_debug when it should have just referred to $this->db_debug.

system/database/drivers/pdo/pdo_result.php 146 Access to an undefined property CI_DB_pdo_result::$db. 148 Access to an undefined property CI_DB_pdo_result::$db. These are actually bugs. This class has no db object nor any db_debug object. This still needs top be fixed. Should we simply echo the stuff? Or not echo it? Throw an exception?

system/database/drivers/pdo/subdrivers/pdo_firebird_forge.php 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. As with the ibase_forge above, this reference to hostname or conn_id or database is not typical of CI3 forge classes. I don’t want to break this so I’ll avoid touching it.

system/database/drivers/pdo/subdrivers/pdo_pgsql_forge.php 102 Access to an undefined property CI_DB_pdo_pgsql_forge::$create_table_if. This looks like a bug, a mistaken reference to $create_table_if instead of $_create_table_if. I have not touched it.

system/database/drivers/pdo/subdrivers/pdo_sqlite_forge.php 133 Access to an undefined property object::$database. This class ultimately inherits $this->db from the class CI_DB_forge, which has $db declared as just an object. We might change this to CI_DB or perhaps CI_DB_query_builder or CI_DB_driver?

        /**
         * Database object
         *
         * @var object
         */
        protected $db;

system/database/drivers/postgre/postgre_driver.php 162 Access to an undefined property CI_DB_postgre_driver::$db. This class CI_DB_postgre_driver extends CI_DB which extends CI_DB_query_builder which extends CI_DB_driver which has a db_debug property and none of these classes has a $db property, so I believe this is a mistake/bug. I have not fixed it because I am not currently able to test postgre, but I believe we should switch $this->db->db_debug to $this->db_debug

system/database/drivers/postgre/postgre_forge.php 90 Access to an undefined property CI_DB_postgre_forge::$create_table_if. I believe this is a bug/mistaken and instead of $this->create_table_if, it should refer to this->_create_table_if

system/database/drivers/sqlite3/sqlite3_forge.php 119 Access to an undefined property object::$database. I believe this is safe, but we could probably eliminate this error by declaring a more specific type for the $db property of the CI_DB_forge class. Perhaps CI_DB?

system/database/drivers/sqlsrv/sqlsrv_driver.php 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. This looks like a bug/mistake. This is the only place in any CI file that refers to _escape_like_chr or _escape_like_str. I have not fixed this. I don’t know what the intent is here.

system/helpers/captcha_helper.php 197 Access to an undefined property CI_Controller::$security. The CI_Controller class declaration is preceded by the #[AllowDynamicProperties] attribute so this should be fine.

system/helpers/cookie_helper.php 74 Access to an undefined property CI_Controller::$input. 92 Access to an undefined property CI_Controller::$input. The CI_Controller class declaration is preceded by the #[AllowDynamicProperties] attribute so these should be fine.

system/helpers/date_helper.php system/helpers/download_helper.php system/helpers/form_helper.php system/helpers/html_helper.php system/helpers/language_helper.php etc etc etc...tons of CI_Controller references should be fine because the CI_Controller class declaration is preceded by the #[AllowDynamicProperties] attribute so these should be fine. Again, we might want to add a $config and $lang property to CI_Controller class to eliminate a lot of these errors.

system/libraries/Image_lib.php 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. I added a $dest_image property in there to get rid of this error.

system/libraries/Migration.php 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. This CI_Migration class has a magic __get method which looks in a CI_Controller for the requested value. This __get method could be improved to throw an exception if the value is not found, but think it’s OK.

system/libraries/Profiler.php 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. This all refer to the $CI controller singleton, which is dynamic, so should be OK? We could get rid of a LOT of these phpstan errors if we declared a $lang property on the CI_Controller class.

I look foward to any feedback and I truly hope my PR can be adopted promptly. I need to get some CI3 sites working with PHP 8.2.

[gxgpet]

Thank you for all your work on this, but this is a no-no...

As I said in my previous comment from #6173, we won't change the entire code base to avoid false positives from a static analysis tool. The code architecture of CI 3 is not built to be automatically reviewed by an algorithm/tool.

Even more, most of those false positives are not even related at all to the PHP 8.2 upgrading issue. They are simply reports of code styling/structure which we prefer/do on the CI 3 code base, and that are not understood (or at least preferred/configured by default) by a tool like phpstan.

Once again - if you find any real PHP 8.2 upgrading issues, you might add/suggest them in #6173, or even create your own PRs with them, sure, but they must be verified and checked that indeed they relate to this new PHP version (and are not just something else).

[sneakyimp]

@gxgpet Closing this PR is a mistake, as it is far better than #6173. If you don't like the change to DB.php, we can roll that back, but I have fixed other bugs, hundreds of bad PHPDoc declarations (which are used by any modern IDE) as well as a few bugs. The reason I created this pull request in the first place was because the lack of progress (over a month) on #6173. I strongly suggest you reconsider. This PR has every change in #6173 and lots of other improvements besides.

[gxgpet]

It's not that I don't like it, it's just that it's not needed. As the title is saying, this PR contains a lot too much to be handled in one PR, if those would be all valid changes.

fixed other bugs

If you find bugs that are centered around one particular issue, even if it's not PHP 8.2 upgrading related, you are more than welcome to open PRs with them, but not a PR to contain all of them, changing the entire code base. You can group them by the code area and/or bug type, it's up to you. But a PR should be addressing one particular matter and be as much as possible easy to review, in the end.

hundreds of bad PHPDoc declarations

Our doc blocks are used solely for documentation purposes, and we won't start changing all of them because of a phpstan run, as I previously said. If it's something that's really bad, we will change it, but otherwise - there wasn't any need to do it until now, and I don't see any now either.

the lack of progress

The lack of progress is because no new reports related to the PHP 8.2 upgrading issues were raised, and when they were and I had enough free time to come back here, I addressed them. Every comment or report made on this was verified and then a specific fix was pushed. No change was automatic.

This PR has every change in #6173 and lots of other improvements besides.

Sorry, but we are not doing extensions to PRs via other PRs... this makes the issue to be hardly tracked and parts of its history can be lost. If you find anything you think we could add on #6173, please address them there, or if you want to make the PR by yourself with other upgrading issues you may find (besides what we have already), then you are welcome to do so too.

[sneakyimp]

@gxgpet

It's not that I don't like it, it's just that it's not needed. As the title is saying, this PR contains a lot too much to be handled in one PR, if those would be all valid changes.

If you were actually to examine the changes, you'd see that the actual functional changes are extremely few. If you don' tlike the change to system/database/DB.php, you needn't adopt it. Personally, I think the existing declaration of CI_DB, is very poor style, and should surely be changed.

If you find bugs that are centered around one particular issue, even if it's not PHP 8.2 upgrading related, you are more than welcome to open PRs with them, but not a PR to contain all of them, changing the entire code base.

The vast majority of changes I've made are to comments only. It is a mischaracterization to say I've changed "the entire codebase."

Our doc blocks are used solely for documentation purposes...

doc blocks are used by modern IDEs to provide developers with contextual autocomplete functionality which helps to avoid a lot of mistakes. I concede that it might be appropriate to fix these docblock errors in a separate PR, but insist that they need to be fixed.

The lack of progress is because no new reports related to the PHP 8.2 upgrading issues were raised...

Indeed, it didn't seem like anyone was making any effort to track down references to the dynamic properties which PHP 8.2 deprecates. This is what prompted me to use a static analysis tool like PHPStan. I'm pleased to see that you are finally making these changes. We haven't seen you post on the other PR since January 26!

Sorry, but we are not extensions to PRs via other PRs... this makes the issue to be hardly tracked and parts of its history can be lost.

As I described above, this PR is a fork of the current, latest CI3 develop branch (a6faab2). I forked the CI3 develop branch, merged the commits in your PR into it, and then applied my commits on top of that. If you downloaded it and examined it, you could see that the entire history is pretty clean.

If you find anything you think we could add on https://github.com/bcit-ci/CodeIgniter/pull/6173, please address them there

I hope you can appreciate that your rejecting my hard work has dampened my enthusiasm to continue work on this. I provided a detailed description above of the changes I made. If you search this page for the word 'bug' you'll see stuff liek this:

system/database/drivers/mysql/mysql_driver.php 157 Access to an undefined property CI_DB_mysql_driver::$db. This looks like it was actually a bug, referring to $this->db->debug when it should have referred to $this->db_debug.

system/database/drivers/mysqli/mysqli_driver.php 230 Access to an undefined property CI_DB_mysqli_driver::$db. Another bug, referring to $this->db->db_debug when it should have just referred to $this->db_debug.

system/database/drivers/pdo/pdo_result.php 146 Access to an undefined property CI_DB_pdo_result::$db. 148 Access to an undefined property CI_DB_pdo_result::$db. These are actually bugs. This class has no db object nor any db_debug object. This still needs top be fixed. Should we simply echo the stuff? Or not echo it? Throw an exception?

I worked hard this weekend to improve this code. It's not very nice of you to reject my hard work and then ask me to do it all over again. I don't get the feeling you've even paid much attention to the nature of changes I've made.

Please, take a closer look.

[RedDragonWebDesign]

I agree in principle with fixing linter errors. Getting rid of the noise in linters allows them to spot actual errors quickly, often as you type in your code editor.

I think it'd be wise to split this into multiple PRs though. One for the docblocks/comments only, one for the CI_DB change, possibly one for each file (since that is how you grouped things). Maybe just do a couple PRs at first, wait for some to be merged, then do some more depending on how things go.

Smaller PRs are less effort to review, allow for easier discussion, allow for quicker merging, etc.

[sneakyimp]

@RedDragonWebDesign

Maybe just do a couple PRs at first, wait for some to be merged, then do some more depending on how things go.

I have been asking questions in #6173 for weeks and had no feedback at all. I then spent many hours fixing bugs this past week, only to have the PR unceremoniously closed by @gxgpet. I am not convinced that any additional work will be accepted based on his comments. Perhaps someone better acquainted with the CodeIgniter devs can break my PR into more manageable pieces.