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

Jump to bottom

Removes requirement of the arguments in the constructor method (Filtered) in favor #615 #616

Merged
merged 8 commits into from
Jun 14, 2014
Merged

Removes requirement of the arguments in the constructor method (Filtered) in favor #615 #616

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
620c2b9
removes requirement of the arguments in the constructor method
hugohenrique May 15, 2014
3aa2e8e
Condition was added to make not mandatory arguments
hugohenrique May 15, 2014
b1ec894
removed unnecessary checks in construct
hugohenrique May 17, 2014
ad8cf09
added the attribution of properties directly, without going through t…
hugohenrique May 17, 2014
b8597a5
added test when no argument is filled
hugohenrique May 17, 2014
ad5e377
replace NotImplementedException to InvalidException as suggestion by …
hugohenrique May 18, 2014
551c5cc
update changes.txt
hugohenrique May 18, 2014
9952a98
fix date changes.txt
hugohenrique May 18, 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
35 changes: 27 additions & 8 deletions lib/Elastica/Query/Filtered.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?php

namespace Elastica\Query;
use Elastica\Exception\NotImplementedException;
use Elastica\Filter\AbstractFilter;

/**
Expand Down Expand Up @@ -33,10 +34,17 @@ class Filtered extends AbstractQuery
* @param \Elastica\Query\AbstractQuery $query Query object
* @param \Elastica\Filter\AbstractFilter $filter Filter object
*/
public function __construct(AbstractQuery $query, AbstractFilter $filter)
{
$this->setQuery($query);
$this->setFilter($filter);
public function __construct(
AbstractQuery $query = null,
AbstractFilter $filter = null
) {
if ($query !== null) {
Copy link
Owner

Choose a reason for hiding this comment

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

The if here is not necessary, because in the toArray it is checked again. Same for the $filter

$this->setQuery($query);
}

if ($filter !== null) {
$this->setFilter($filter);
}
}

/**
Expand Down Expand Up @@ -93,9 +101,20 @@ public function getQuery()
*/
public function toArray()
{
return array('filtered' => array(
'query' => $this->_query->toArray(),
'filter' => $this->_filter->toArray()
));
if ($this->_query === null && $this->_filter === null) {
throw new NotImplementedException('The query or filter have not been defined, you define at least one');
Copy link
Owner

Choose a reason for hiding this comment

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

I would suggest to use the not found exception here.

Also please use an exception per variable so it is clear, which one is missing for the developer using the library.

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 don't understand this point: "Also please use an exception per variable so it is clear, which one is missing for the developer using the library."

Copy link
Contributor

Choose a reason for hiding this comment

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

Judging from the docs, both a query and filter are always required, so if one is missing the user needs to know which. Your message suggests it's possible to supply only one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mal Not! both unnecessary arguments and documentation shows that you can use both "query", "filter" or both.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, you can save a comparison with something like;

    public function toArray()
    {
        $filtered = array();

        if ($this->_query !== null) {
            $filtered['query'] = $this->_query->toArray();
        }

        if ($this->_filter !== null) {
            $filtered['filter'] = $this->_filter->toArray();
        }

        if (empty($filtered)) {
            throw new NotFoundException('A query and/or filter is required');
        }

        return array('filtered' => $filtered);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mal thanks for the suggestion!

}

$filtered = array();

if ($this->_query !== null) {
$filtered['query'] = $this->_query->toArray();
}

if ($this->_filter !== null) {
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of checking for not null, I would suggest to check if it is actually a Filter) (or above a query) as you did before in the constructor.

$filtered['filter'] = $this->_filter->toArray();
}

return array('filtered' => $filtered);
}
}