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.

Sign up for GitHub

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

minor changes in Query class #733

Merged
merged 3 commits into from
Jan 2, 2015
Merged

minor changes in Query class #733

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ec027b
minor changes in Query class
webdevsHub Dec 14, 2014
40eb3e5
removed typehint
webdevsHub Dec 22, 2014
a00191f
updated changes.txt
webdevsHub Dec 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions lib/Elastica/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Elastica\Query\AbstractQuery;
use Elastica\Query\MatchAll;
use Elastica\Query\QueryString;
use Elastica\Rescore\AbstractRescore;
use Elastica\Suggest\AbstractSuggest;
use Elastica\Suggest;

Expand Down Expand Up @@ -393,11 +394,12 @@ public function setSuggest(Suggest $suggest)
/**
* Add a Rescore
*
* @param \Elastica\Rescore\AbstractRescore $suggest suggestion object
* @param \Elastica\Rescore\AbstractRescore $rescore suggestion object
* @return \Elastica\Query Current object
*/
public function setRescore($rescore)
public function setRescore(AbstractRescore $rescore)
Copy link
Contributor

Choose a reason for hiding this comment

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

You realize that by adding a new typehint to class that somebody might be extending is a BC break? Adding typehints is a good thing, but then the release process should take that into account (=change like this should not appear in patch version release and ideally not even in feature version release).

Copy link
Owner

Choose a reason for hiding this comment

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

@webdevsHub I suggest you mention this in the changes.txt as a BC break.

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 just thought it would be better then getting a Fatal Error on
$rescore->toArray() line.

What do you suggest to do to take the release process into account?
Am 14.12.2014 18:36 schrieb "Filip Procházka" [email protected]:

In lib/Elastica/Query.php
#733 (diff):

  */
  • public function setRescore($rescore)
  • public function setRescore(AbstractRescore $rescore)

You realize that by adding a new typehint to class that somebody might be
extending is a BC break? Adding typehints is a good thing, but then the
release process should take that into account (=change like this should not
appear in patch version release and ideally not even in feature version
release).

—
Reply to this email directly or view it on GitHub
https://github.com/ruflin/Elastica/pull/733/files#r21797723.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can always do "manual check"

if (!$rescore instanceof AbstractRescore) {
    throw new \LogicException(sprintf('Instance of AbstractRescore expected, but %s was given.', get_type($rescore)));
}

return $this->setParam('rescore', $rescore->toArray());

About the release process, it doesn't matter if change like this is in master branch, but it does matter if it's in some release-branch and if it it's tagged.

{
$this->setParam('rescore', $rescore->toArray());
return $this->setParam('rescore', $rescore->toArray());
}

/**
Expand All @@ -416,7 +418,7 @@ public function setSource(array $fields)
* Sets post_filter argument for the query. The filter is applied after the query has executed
*
* @param array|\Elastica\Filter\AbstractFilter $filter
* @return \Elastica\Param
* @return \Elastica\Query Current object
* @link http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/search-request-post-filter.html
*/
public function setPostFilter($filter)
Expand Down