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

initial commit of code changes to add ResultInterface::getNumRows method #4047

Closed
wants to merge 1 commit into from
Closed

initial commit of code changes to add ResultInterface::getNumRows method #4047

wants to merge 1 commit into from

Conversation

sneakyimp
Copy link
Contributor

Also adds the implementation of this method to the various DBMS-specific Result classes. Also added unit test tests/system/database/Live/GetNumRowsTest for this newly added method.

Each pull request should address a single issue and have a meaningful title.

Description
Per Lonnie Ezell's message in #109, I have attempted my first CodeIgniter fork/branch/pull request here. It adds a getNumRows method to the ResultInterface and the various DBMS classes that extend it. I've not yet updated the user guide but will try to do so if this looks good.

Checklist:

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

…abase/ResultInterface and the various DBMS-specific Result classes. Also added unit test in the database/Live folder for this newly added method.
@sneakyimp
Copy link
Contributor Author

sneakyimp commented Jan 1, 2021

OK wow so the automated tests are failing like crazy, but the errors I see don't appear to relate to any files that I've changed. I'm quite new to pull requests and these automated tests. I do know that the one test I added does run without error on my workstation. @lonnieezell perhaps you could help me understand how to pass these tests?

EDIT: something seems off as well about the extent of changes I've made. I think I might close this pull request and try to make a cleaner one.

@sneakyimp
Copy link
Contributor Author

I'm going to close this PR and make a cleaner one.

@sneakyimp sneakyimp closed this Jan 1, 2021
@sneakyimp sneakyimp deleted the feature/num-rows-signed branch January 1, 2021 01:13
/**
* Retrieve the results of the query. Typically an array of
* individual data rows, which can be either an 'array', an
* 'object', or a custom class name.
*
* @param string $type The row type. Either 'array', 'object', or a class name to use
*
* @return array
* @return mixed
Copy link
Member

Choose a reason for hiding this comment

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

Why changed to mixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned elsewhere, this first attempt at a PR had outdated files as its starting point. That is why I closed this request and tried again. Thank you for look at this PR, but I want you to know that I've closed it and it should be ignored. I'm sorry for wasting your time :(

* @param string $key
* @param null $value
* @param $key
* @param null $value
Copy link
Member

Choose a reason for hiding this comment

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

string is gone.

@kenjis
Copy link
Member

kenjis commented Jan 1, 2021

@sneakyimp

PHP Fatal error: Class CodeIgniter\Test\Mock\MockResult contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (CodeIgniter\Database\ResultInterface::getNumRows) in /home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Mock/MockResult.php on line 108
https://github.com/codeigniter4/CodeIgniter4/pull/4047/checks?check_run_id=1631979520

You changed ResultInterface, so you have to add getNumRows() to MockResult.
Because MockResult extends BaseResult, and BaseResult implements ResultInterface.

@@ -58,47 +87,51 @@ public function getFieldNames(): array
*/
public function getFieldData(): array
{
static $dataTypes = [
static $data_types = [
Copy link
Member

Choose a reason for hiding this comment

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

Why snake_case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had mistakenly copied MySQLi/Result.php from a project I've been working on for a few weeks and I guess there were extensive edits to the comments (and this change of var name) in the interim. It became apparent after I submitted this PR that I needed to use more up-to-date files. That is why I closed this PR.

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