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

Null Handling #6

Closed
MGatner opened this issue Aug 14, 2021 · 4 comments · Fixed by #14
Closed

Null Handling #6

MGatner opened this issue Aug 14, 2021 · 4 comments · Fixed by #14

Comments

@MGatner
Copy link
Member

MGatner commented Aug 14, 2021

Your latest commit points out a problem in the current flow: settings with null values. I think this will be a frequent enough need with Config handling that it will merit handling explicitly (maybe types will help?). Handlers should be able to:

  • Handle a set() request to store an overriding null value
  • Differentiate "no handler value" that should percolate from an actual null value response from get() requests that should stop

The service and helper will need updating to accommodate these parameters.

@lonnieezell
Copy link
Member

lonnieezell commented Aug 16, 2021

I scanned through all of the config files in CodeIgniter and only 3 files had null values: ContentSecurityPolicy, Email, and Kint. All of those could be handled with empty strings, though obviously we wouldn't want to do that now.

My point, though, is that I don't think null values would be used all that much - forgetting the saved setting and letting it revert back to it's original (or saving an empty string/boolean false) would be much more common.

My thinking here is:

  1. The settting() helper method stays the same. Converting that to work with a null value (which currently is how it knows whether it's getting or setting) would be more trouble than it seems to be worth.
  2. Ensure that the set() method on the Settings class can handle null values so that if someone truly needs it, they can always do it with: setting()->set('foo', null);
  3. Ensure the database table supports a null value field.

Thoughts?

@MGatner
Copy link
Member Author

MGatner commented Aug 16, 2021

I would still push for full null compliance. I think much of the framework has not handled nullables very well, and they will only get more important once we drop 7.3 since we will be able to add actual types to everything. Things like Logger Config using this is poor:

   * Note: Leaving it blank will default to 'log'.
   */
  'fileExtension' => '',

Even if null values aren't common in the framework they are in libraries. I personally use them heavily; Myth:Auth uses them at very key control points. Empowering these from the service but not the helper seems unduly punishing.

@MGatner
Copy link
Member Author

MGatner commented Aug 16, 2021

I think the helper would actually be an easy fix using func_get_args(): https://3v4l.org/L2ATH

I can work on a PR if you are amenable to head this direction.

@lonnieezell
Copy link
Member

Oh, interesting, I didn't realize that would work. I guess it makes sense, but I thought with the default being null it wouldn't recognize. Cool. The only other way I was thinking would work well would be another class to represent null, or something, which seemed pretty ugly.

Yes, I'm good with that approach.

MGatner added a commit that referenced this issue Nov 11, 2021
@MGatner MGatner linked a pull request Nov 11, 2021 that will close this issue
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 a pull request may close this issue.

2 participants