Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP8.4 deprecation notices fix attempt #1

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

NielBuys
Copy link
Owner

Attempt to Fix below issues
bcit-ci#6302
bcit-ci#6300
bcit-ci#6300

@NielBuys
Copy link
Owner Author

Changes not tested on php8.4 yet. But it should remove notices. Changes works on my php8.3 environment.

@jamieburchell
Copy link

jamieburchell commented Oct 8, 2024

Should probably bump the required PHP version to 7.1 if using nullable types. I personally don't care about anything < PHP 8.2, but I'm not sure if these PRs are supposed to be compatible with the PHP version mentioned in composer.json.

@jamieburchell
Copy link

jamieburchell commented Oct 8, 2024

Won't there still be deprecation notices in PHP 8.4 because session.sid_length is still being used?

@NielBuys
Copy link
Owner Author

NielBuys commented Oct 9, 2024

Should probably bump the required PHP version to 7.1 if using nullable types. I personally don't care about anything < PHP 8.2, but I'm not sure if these PRs are supposed to be compatible with the PHP version mentioned in composer.json.

On my fork I have bumped the minimum php version to 7.4. That's the reason I have not pushed this pull request to the main repository.

@NielBuys
Copy link
Owner Author

NielBuys commented Oct 9, 2024

Won't there still be deprecation notices in PHP 8.4 because session.sid_length is still being used?

Thanks for the heads up. I haven't tested my changes with PHP 8.4 yet; I've only researched and made the updates. I'll look into session.sid_length and set up a test environment for PHP 8.4.

@NielBuys
Copy link
Owner Author

NielBuys commented Oct 9, 2024

Won't there still be deprecation notices in PHP 8.4 because session.sid_length is still being used?

The session.sid_length section of the code was sourced from the pull request (codeigniter4/CodeIgniter4#9139) for CodeIgniter 4, which was referenced in issue bcit-ci#6300.

@jamieburchell
Copy link

The session.sid_length section of the code was sourced from the pull request (codeigniter4/CodeIgniter4#9139) for CodeIgniter 4, which was referenced in issue bcit-ci#6300.

Yup, that forces the params for <PHP9, but it will still trigger a deprecation error. CI4 gets around that in a different way

@jamieburchell
Copy link

jamieburchell commented Oct 9, 2024

At this point and assuming it stops the deprecation notice, I think I'd be tempted to simply prefix the ini_set call for session.sid_length and session.sid_bits_per_character with the error suppression character '@'.

@NielBuys
Copy link
Owner Author

NielBuys commented Oct 9, 2024

At this point and assuming it stops the deprecation notice, I think I'd be tempted to simply prefix the ini_set call for session.sid_length and session.sid_bits_per_character with the error suppression character '@'.

After reviewing it, I believe you're right; it will still trigger the deprecation notice. I think it’s a good idea to add the @ symbol in front of ini_set. I will make that change and then test to see if it works.

@jamieburchell
Copy link

@NielBuys I was feeling brave and decided to go all-out in my branch and remove code supporting < PHP 7.4. It might benefit you and your repo. Comments welcome too!

@NielBuys
Copy link
Owner Author

NielBuys commented Oct 9, 2024

@NielBuys I was feeling brave and decided to go all-out in my branch and remove code supporting < PHP 7.4. It might benefit you and your repo. Comments welcome too!

Thanks, I agree that it's a good idea to clean up some of the older PHP version code. I've created a patch file from your commit and will review it. I’ll let you know if I encounter any issues.

@NielBuys NielBuys merged commit fc90b48 into develop Dec 20, 2024
@NielBuys NielBuys deleted the tasks/php8_4_deprecation_notices branch December 20, 2024 07:29
@rhickmott
Copy link

Should probably bump the required PHP version to 7.1 if using nullable types. I personally don't care about anything < PHP 8.2, but I'm not sure if these PRs are supposed to be compatible with the PHP version mentioned in composer.json.

It can still be backwards compatible. I don't think there's really that many nullable types in Code Ignitor but this error can be removed by removing the types from the very few functions that use them to maintain backwards compatibility.
Optionally where needed, a check can be made in code to validate or cast to the required typing or check for nulls.

Won't there still be deprecation notices in PHP 8.4 because session.sid_length is still being used?

Only if you directly try to set these parameters using ini_set.
You can check the version for PHP easily in the offending method.
The code "being there" isn't the problem unless it's a function being removed and then you an add in a dummy function if it doesen't exist in the base language.

elseif( PHP_VERSION_ID >= 80400 ) {
#PHP 8.4 REMOVES SETTING OF BIRS PER CHARACTER AND SESSION LENGTH
$bits_per_character = 4;
$sid_length = 32;
}

There is already code in system\librarys\Session\Session for handling legacy versions prior to 7.0.1 in this manor.

The above are the defaults for the session ID which have been 32 Character SSID ( 160bit ) since at least PHP 7.0.1 judging by the code. It looks like the plan it to enforce this default going forward and by the comments it looks like CI was happy with this setting.

// Add as many more characters as necessary to reach at least 160 bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants