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

Add magic __isset to classes with __get #2231

Merged
merged 6 commits into from
Sep 19, 2019

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Sep 17, 2019

Description
See #2219

This PR adds isset() methods to the following classes, modeled after each class's __get() method:

  • CodeIgniter4/system/Database/BaseConnection.php
  • CodeIgniter4/system/Model.php
  • CodeIgniter4/system/Session/Session.php
  • CodeIgniter4/system/I18n/Time.php
  • CodeIgniter4/system/CLI/BaseCommand.php
  • CodeIgniter4/system/I18n/TimeDifference.php
  • CodeIgniter4/system/Encryption/Encryption.php
  • CodeIgniter4/system/Encryption/Handlers/BaseHandler.php

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

@lonnieezell or @jim-parry I don't foresee any issues with this but would still appreciate a glance.

@jim-parry
Copy link
Contributor

Weird that the same purpose method has to be done so differently across the classes; suggests that it is a good idea.
And code coverage went down because of new code that isn't explicitly tested, I gather :-/

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

Yeah, it's a bummer but PHP uses __isset() natively when __get() is defined since it's valid for __get() to throw: https://www.php.net/manual/en/language.oop5.overloading.php#object.isset

I suppose we could add tests for each class that received the new magic function, but might make more sense to find a way to exempt them?

EDIT: Exemption is silly, because they could easily have failures. Let me know if you'd prefer a MagicIssetTest.php prior to merge.

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

Actually something seems to be buggy (or I'm ignorant of something) with coverage tests right now. See for example:

@jim-parry
Copy link
Contributor

That would make sense, or adding them to existing tests. The troublesome one, IMHO, is the Model, as the property could be interpreted in different ways. Is it a problem if two or more of the possible properties exist?

@MGatner
Copy link
Member Author

MGatner commented Sep 17, 2019

The model __isset() follows the test pattern of __get(), so it's possible there's a logic problem but it would be much larger than this new method :). I think though that isset() is intentionally ignorant of its content so it's pretty safe to have extra successes and the follow-up __get() would be the place for validating.

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

I believe they all look fine. I had one comment on the Model that I think we need to consider. Part of me is wondering if the session handler needs to handle some of the other situations, like flash data or temp data, but i can't recall exactly how they're stored if they'd need a special case for that or not.

Also - yes , please add a tests suite for these. Or add to existing tests.

system/Model.php Show resolved Hide resolved
@MGatner MGatner merged commit 957d674 into codeigniter4:develop Sep 19, 2019
@MGatner MGatner deleted the magic-issets branch September 19, 2019 16:16
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