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 property_exists check on BaseBuilder and BaseConnection for $this->$key set value #1522

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

samsonasik
Copy link
Member

To avoid create non-existente property values.

Checklist:

  • Securely signed commits

@jim-parry
Copy link
Contributor

I'm not sold on this one. The original lets you inject properties into a QB at construction time, while the proposed change only lets you override them.
The block of code is not unit tested, nor can I find anything about it in the user guide, so there is no guidance as to the intent of this parameter.
Maybe it should be eliminated altogether.
@lonnieezell Can you shed light on this?

@lonnieezell
Copy link
Member

The database stuff is something I never did huge modifications to and so a lot of its inner workings is still a bit unknown to me. That's directly from CI3 code, though. That's how the configuration is passed to the driver.

From what I can tell the intent was to override, not inject in the first place, but I definitely haven't looked at all of the drivers we don't support. I don't see anything listed in the CI3 user guide about extra params for any of the databases there, so I think it's safe to assume override is all that is needed.

@samsonasik
Copy link
Member Author

samsonasik commented Nov 27, 2018

with assumption, auto create class property field is allowed, it still needs a validation then, eg, when passed options has "numeric" key, ref https://3v4l.org/o4IRj that will make error

@lonnieezell
Copy link
Member

My assumption was that we were overriding only, not creating new class properties.

@samsonasik
Copy link
Member Author

so, the property_exists() check is correct?

@lonnieezell
Copy link
Member

@samsonasik Yes, I believe so. Leaving this for @jim-parry to finalize on since he requested my opinion on this.

@jim-parry jim-parry merged commit 48fc063 into codeigniter4:develop Nov 28, 2018
@samsonasik samsonasik deleted the property-exists-key-set branch November 28, 2018 11:20
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.

None yet

3 participants