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

fix for php 8.3 #510

Closed
wants to merge 22 commits into from
Closed

Conversation

alexandergotts
Copy link
Contributor

updates codeigniter/framework to be compatible for php 8.2+ and removes fix that no longer works

composer.json Outdated Show resolved Hide resolved
@tenzap
Copy link
Collaborator

tenzap commented Feb 13, 2024

Please check #512
Is your PR still required?

@alexandergotts
Copy link
Contributor Author

Please check #512 Is your PR still required?

I think my pr is no longer needed

tenzap
tenzap previously approved these changes Feb 14, 2024
@tenzap
Copy link
Collaborator

tenzap commented Feb 14, 2024

When doing squash & merge, use this commit message:

remove deprecated call to CI_Model::__construct()

In future CI3 version 3.2.0, that constructor is entirely removed,
which would result in fatal errors on attempts to call it.

See: https://github.com/bcit-ci/CodeIgniter/blob/develop/user_guide_src/source/installation/upgrade_320.rst

@alexandergotts
Copy link
Contributor Author

get_class() not working with php 8.3

@@ -30,7 +30,6 @@ class Sms_credit_model extends CI_Model {
*/
function __construct()
{
parent::__construct();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, and a few others, it seems it is advised to remove the constructor completely. See comment here: https://github.com/bcit-ci/CodeIgniter/blob/develop/user_guide_src/source/installation/upgrade_320.rst#step-3-remove-calls-to-ci_model__construct
There are other similar ones below.

@@ -43,7 +43,7 @@ function __construct($login = TRUE)
$this->plugin_name = strtolower(get_class($this));

// Prevent this controller from being called directly
if (get_class() === get_class($this))
if ("Plugin_controller" === get_class($this))
Copy link
Collaborator

@tenzap tenzap Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using __CLASS__ instead? Any advantage of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works too. i have changed it

@kingster
Copy link
Collaborator

Shouldn't we remove the construct function itself? If there is nothing inside it?

@tenzap
Copy link
Collaborator

tenzap commented Feb 14, 2024

Shouldn't we remove the construct function itself? If there is nothing inside it?

That's what I meant in my comment above #510 (comment)

@alexandergotts
Copy link
Contributor Author

Could work but it also works if you just leave it empty

@tenzap
Copy link
Collaborator

tenzap commented Feb 16, 2024

Could you please squash all the commits to leave only these two and set an appropriate commit message?

  • one commit for the changes related to the constructor for CI3.2. I think it is cleaner to remove completely the empty constructors.
  • one commit for the changes related to php 8.3 (__CLASS__)

@alexandergotts
Copy link
Contributor Author

Could you please squash all the commits to leave only these two and set an appropriate commit message?

  • one commit for the changes related to the constructor for CI3.2. I think it is cleaner to remove completely the empty constructors.
  • one commit for the changes related to php 8.3 (__CLASS__)

I can only make one fork that I can commit

@tenzap
Copy link
Collaborator

tenzap commented Feb 16, 2024

You probably need to use the command line for that.

@tenzap
Copy link
Collaborator

tenzap commented Feb 16, 2024

Search for "git squash commit" in a search engine.

@alexandergotts
Copy link
Contributor Author

it's done

@tenzap
Copy link
Collaborator

tenzap commented Feb 17, 2024

Thanks

@tenzap
Copy link
Collaborator

tenzap commented Feb 17, 2024

Replaced by #513 & #514

@tenzap tenzap closed this Feb 17, 2024
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