Adding PHP 8.2 support #6173
Adding PHP 8.2 support #6173
Conversation
|
Thank you, @gxgpet. In addition to the changes your are proposing, I had to add |
|
Thanks for your suggestion, @Khuthaily! I added the attribute only for |
|
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. |
|
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.txtThis yields about 2090 errors (see attached phpstan-output.tgz), including 688 'Access to an undefined property' errors. One example of which is:
I grepped those lines and wrote a quick PHP script to list all the distinct classes before the '::' and this is the result: 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 |
|
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: 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? |
|
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.txtThis is still reporting about 1000 errors, but only 54 of those refer to undefined properties: 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 /*
* ------------------------------------------------------
* 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 /**
* ------------------------------------------------------
* 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 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? |
|
I'd very much like to see PHP 8.2 supported... |
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.
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:
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.
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. |
|
As I said last week:
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. |
|
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. |
|
I couldn't figure out how to use my application without these errors |
|
someone to help me? |
|
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.
My app works in my test environment. Here is the patch file I created from this pull request for record purposes. Thanks all |
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. |
|
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. |
|
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? |
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. |
|
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). |
|
Will this branch be merged? I am looking forward to see php 8.2 support for CI3. |
|
@narfbg Are you able to merge this pull request? |
No issues with this PR. I have it merged in to a fork we are using in production along with some other fixes. You can see the difference between the fork and 3.1.13 here. |
|
I have come across an issue in my tests - although I respect this is a little difficult to replicate due to the hardware stack, thus a bit of an edge case. It's one of that which I've identified the issue but would appericate input if the fix could break other aspects of the query builder. Our site uses Oracle OCI8 & MySQL DB instances. See below for a small example: $odbc = $this->load->database( "oracle", true );
$mysql = $this->load->database( "mysql", true );
$products = $odbc->select([ "sku", "stock" ])->from( "products" )->get_compiled_select();
$users = $mysql->select([ "username", "dt_last_login" ])->from( "users" )->get_compiled_select();
echo PHP_VERSION . "<br />";
echo $products . "<br />";
echo $users;Running the above in 8.0.24
SELECT "sku", "stock" FROM "products"
SELECT `username`, `dt_last_login` FROM `users`However in 8.1.28
SELECT "sku", "stock" FROM "products"
SELECT "username", "dt_last_login" FROM "users"This produces a invalid MySQL query and thus a hard MySQL Error. Note the The offending line is in CodeIgniter/system/database/DB_driver.php Line 1417 in 6074d34 Simply removing |
I think it's this Not sure why that |
|
This might be useful for you. codeigniter4/CodeIgniter4#5262 |
I'll open a PR for this later unless @gxgpet is planning on doing the same. |
|
@mackieee Would you mind testing this PR to see if this resolves your issue? Please note that the PR is based from the develop branch and so merging only these changes in to the master/latest release needs to be done carefully (there will be some conflicts) so as not to copy over anything else that's changed. I'll merge this in to my fork if there are no issues found. |
|
Wow, thanks for your time and effort here @jamieburchell 🥇 Highly appericated. I'll give this a test now :) Update: That worked a treat 👍 As per your note on the usage of the static variables in the final classes, doesn't appear to have been removed in @kenjis's review on this in CI4 and is still current repo. So I'd say we're safe there. |
|
Came across this issue a while back but didn't have time to address it. The app I inherited was actually still running with CI2 and I didn't really see how to upgrade to CI4 which supposedly fixed the issue. Of course that turns out to be a no go completely but I now succeeded in getting it to the latest CI3 release. Definitely an improvement with respect to how the warnings are displayed in debug mode but the documentation on PHP appears to not be created for human reading and it took me at least an hour to conclude that you are supposed to declare a variable before using it. My other understanding was that I might circumvent the issue by declaring the classes with You have added |
Don't think that's correct. Example being when you load a library and reference it as |
That is what I initially thought as well, but on second view I noticed that the warnings still produced stated an origin address in |
I don't have it and I'm not seeing any warnings. What you need to do here is that if you want to use class Foo as |
You're free to do that, but I haven't encountered any CI projects where the libraries loaded are defined as controller class variables in that way. I'd say at the very least it wasn't typical for CI. By allowing dynamic variables, it provides PHP 8.2 compatibility for existing codebases that use CI libraries in the documented and typical way, without any effort. |
That is what I said. Adding this attribute allows running old code that as of PHP 8.2+ is essentially bad. I understand how that is appealing but I don't think it is the responsibility of the core library to mask such bad code, nor to encourage people to create new bad code based on this functionality. |
But this is how to do things in CI3. It's an ancient framework with its roots in ancient versions of PHP with compatibility maintained for those versions. There is nothing modern about CI3 in terms of PHP. That's what CI4 is for. These fixes are for desperate life support only, not for rethinking the framework. It's also not really practical to define the libraries that might be loaded at any given time from any place in the application. Consider this code in some random library or helper somewhere: |
If that is the objective you shouldn't upgrade PHP but run it in a hypervisor if hardware becomes an issue for the corresponding old OS |
Or just add a single comment to the CI_Controller class to maintain compatibility with how the framework is intended to be used for PHP 8.2+ 🤷🏻♂️ |
Sure. You can do that. But on an individual base. |
I disagree because this isn't a problem introduced by users writing sloppy code, rather a result of how the framework is structured to allow dynamic variables to be attached to the CI instance. And because the CI instance can be grabbed from any library or helper (or anywhere really) it's not feasible to pre-define every library that might be injected in to the CI instance. Much the same reason why this fix is required for the CI Loader. Of course, if the framework had done things differently in the first place perhaps by making use of a class array to hold these things, with magic getters and setters, dynamic properties wouldn't be needed at all. But like I say, this is to appease how the framework is designed without reengineering it. |
|
It's not the CI Instance that made you create a class like In fact I'm sure that you already had to change |
|
I don't see what class constructors have to do with this issue. Simply put, the CI framework makes use of dynamic properties in CI_Controller (which is the CI super instance). You can see more examples in the framework code. Same as for CI_Loader |
|
It's not about the class constructor. It's about referencing |
|
This has already been discussed. The fix decided upon is the use of the comment attribute in this PR. It's almost certainly a moot point anyway; I don't believe there will ever be a merge of this to create a new release of CI3. Fork the framework and modify as you see fit, or use a fork that aligns better with your expectations. Better, if you care about best practice and modern PHP, use a modern framework. There are many, many aspects of CI3 that are no longer best practice in modern PHP - dynamically created class variables are only one example. |
|
@jamieburchell Might want to have a look how this PR addresses named issue with the very file mentioned in that link you provided. |
|
When this will merge in official base branch? |
This adds support for PHP 8.2.
Basically, fixing dynamic props assigning.