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

New method Find Column w.r.t. #1619 #1861

Merged
merged 9 commits into from Apr 1, 2019
Merged

New method Find Column w.r.t. #1619 #1861

merged 9 commits into from Apr 1, 2019

Conversation

atishhamte
Copy link
Contributor

@atishhamte atishhamte commented Mar 23, 2019

W.r.t #1619, correction in the codes and introduced test cases as well

Its a shorthand query.

Checklist:

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

@lonnieezell
Copy link
Member

I believe the purpose of the originally proposed method was to return an array of the values only, at least when a single column was asked for, like:

$job = $model->findColumn('name');
$this->assertEquals('Developer', $job[0]);
$this->assertEquals('Politician', $job[1]);
$this->assertEquals('Accountant', $job[2]);
$this->assertEquals('Musician', $job[3]);

That makes it easy to grab a column of values and then immediately use it for a whereIn, or whatever.

@atishhamte
Copy link
Contributor Author

atishhamte commented Mar 25, 2019

I kept the purpose is to shorthand the query, where you can get the response in the format of object or array when you pass single and multiple columns.
But the response format will remain the same.
If I make any changes w.r.t. only array and someone wants a response in an object, then parsing is the only option to do so.
Also, there would be a confusion of the return type of single and multidimensional array w.r.t. single and multiple columns respectively.

@lonnieezell
Copy link
Member

My personal feeling is that if all it does is provide a shorthand way to get rid of doing ->select('x') then it's not a feature I'm interested in. Returning an array of values is something I can see being a time saver for a lot of people so I'm ok with that.

With that in mind - I think it should always return an array of the actual values. If need be, restrict it to a single column name.

@atishhamte
Copy link
Contributor Author

No problem. I will make the necessary changes

@atishhamte
Copy link
Contributor Author

Does the second parameter is required in the method? or we should strictly keep it as a result set of all values under the column without where condition.

@atishhamte
Copy link
Contributor Author

I have done the changes for single column input along with exception handling

@atishhamte
Copy link
Contributor Author

@lonnieezell and @jim-parry, any changes required in this PR?

system/Model.php Outdated Show resolved Hide resolved
@atishhamte
Copy link
Contributor Author

Changes done according to array_column

@atishhamte
Copy link
Contributor Author

@lonnieezell any changes required here?

@lonnieezell
Copy link
Member

Looks good! Thanks.

@lonnieezell lonnieezell merged commit b1820ed into codeigniter4:develop Apr 1, 2019
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

2 participants