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

refactor: remove unneeded Entities\Cast\IntBoolCast #1186

Closed
wants to merge 1 commit into from

Conversation

grimpirate
Copy link
Contributor

Description
While looking at the shield source code it was noted that the IntBoolCast class is already provided by codeigniter. Furthermore, the int_bool cast handler of the custom shield Entity already provided as an int-bool in the framework's default Entity. This commit cleans up redundant code.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

I’m not entirely sure, but as far as I remember, these features were added to the framework after they were identified as necessary in Shield. Since Shield also supports older versions of the framework, we need to ensure that these changes do not break compatibility.

Could you please investigate which version of the framework these features were introduced in and make sure that the proposed changes will not cause issues with older versions?

@kenjis kenjis changed the title Unneeded/duplicate pieces of code from main framework refactor: Unneeded/duplicate pieces of code from main framework Aug 27, 2024
@kenjis kenjis added the refactor Pull requests that refactor code label Aug 27, 2024
@kenjis
Copy link
Member

kenjis commented Aug 27, 2024

Thank you for sending this PR.
Also, please fix all errors in GitHub Actions.

@grimpirate
Copy link
Contributor Author

You were correct regarding supporting older framework it appears datamweb: see here and here. Feature was implemented approximately two years ago.

@grimpirate grimpirate closed this Aug 27, 2024
@kenjis
Copy link
Member

kenjis commented Aug 27, 2024

@datamweb

We have phpunit test "phpunit / PHP 7.4 - MySQLi - lowest" that installs the lowest dependencies.
But it installs v4.4.7.

  • Installing codeigniter4/framework (v4.4.7): Extracting archive

Ah, that is because v4.4.6 or below has one or more known vulnerabilities.
https://github.com/codeigniter4/CodeIgniter4/security/advisories

@kenjis
Copy link
Member

kenjis commented Aug 27, 2024

@grimpirate See the version tags.
Screenshot 2024-08-27 10 30 15
codeigniter4/CodeIgniter4@899187a

@kenjis
Copy link
Member

kenjis commented Aug 27, 2024

@grimpirate Why did you close this?
IntBoolCast has been introduced in v4.3.0, and Shield requires v4.3.5 or later.
See https://shield.codeigniter.com/getting_started/install/#requirements

@kenjis kenjis changed the title refactor: Unneeded/duplicate pieces of code from main framework refactor: remove unneeded Entities\Cast\IntBoolCast Aug 27, 2024
@grimpirate
Copy link
Contributor Author

@kenjis sorry about that I thought based on the commit history that it was for backwards compatibility. I didn't think to compare the version numbers.

@kenjis
Copy link
Member

kenjis commented Aug 27, 2024

@grimpirate Okay, no problem. As you see, your refactoring is okay.
So if you send the PR again, we would review and merge.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants