Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Conversation

@lowtower
Copy link
Contributor

@lowtower lowtower commented Apr 11, 2018

Change Return Type of selectWith() method

Provide a narrative description of what you are trying to accomplish:

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.

phpstan raises an error saying that ResulSetInterface has no method current().

  • Detail the original, incorrect behavior.

The selectWith() method has defined the @returnType as ResultSetInterface.
The method calls the method executeSelect(), which returns ResultSet.
The return types should be the same.
removes also the @throws flag as it doesn't throw an exception itself, but just the called methods.

  • Detail the new, expected behavior.

  • Base your feature on the master branch, and submit against that branch.

  • Add a regression test that demonstrates the bug, and proves the fix.

  • Add a CHANGELOG.md entry for the fix.

  • Are you creating a new feature?

    • Why is the new feature needed? What purpose does it serve?
    • How will users use the new feature?
    • Base your feature on the develop branch, and submit against that branch.
    • Add only one feature per pull request; split multiple features over multiple pull requests
    • Add tests for the new feature.
    • Add documentation for the new feature.
    • Add a CHANGELOG.md entry for the new feature.
  • Is this related to quality assurance?

  • Is this related to documentation?

Change Return Type of selectWith() method
@tptrixtop
Copy link
Contributor

May be use then AbstractResultSet in lieu of ResultSet ?

@samsonasik
Copy link
Contributor

selectWith() can return HydratingResultSet, so AbstractResultSet is better.

@lowtower
Copy link
Contributor Author

Then select(), selectWith() and executeSelect() should all return AbstractResultSet.
Right?

@samsonasik
Copy link
Contributor

yes

@froschdesign
Copy link
Member

Why AbstractResultSet? Compare with:

/**
* @var ResultSetInterface
*/
protected $resultSetPrototype = null;

if (! $this->resultSetPrototype instanceof ResultSetInterface) {
$this->resultSetPrototype = new ResultSet;
}

// build result set
$resultSet = clone $this->resultSetPrototype;
$resultSet->initialize($result);
// apply postSelect features
$this->featureSet->apply(EventFeatureEventsInterface::EVENT_POST_SELECT, [$statement, $result, $resultSet]);
return $resultSet;

@samsonasik
Copy link
Contributor

missing use statement:

use Zend\Db\ResultSet\AbstractResultSet;

@samsonasik
Copy link
Contributor

samsonasik commented Apr 11, 2018

@froschdesign how about using @var ResultSetInterface|AbstractResultSet for $resultSetPrototype property ?

@lowtower
Copy link
Contributor Author

@froschdesign
indeed, not consistant!
What do you propose?

@ezimuel
Copy link
Contributor

ezimuel commented Aug 7, 2018

@samsonasik Zend\Db\ResultSet\AbstractResultSet implements Zend\Db\ResultSet\ResultSetInterface, it's better to use this interface as return type.
@lowtower can you propose a new PR chaning all the types to ResultSetInterface? Thanks!
I'm closing this PR in favor of a new one.

@ezimuel ezimuel closed this Aug 7, 2018
@lowtower lowtower mentioned this pull request Aug 7, 2018
17 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants