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

fix redis + where + pageError #792

Closed
wants to merge 3 commits into from

Conversation

jinmarcus
Copy link

@jinmarcus jinmarcus commented Oct 24, 2017

fix redis + where + pageError

The error thrown is always:pageNotFound and fixed it
The following is wrong:

Argument 1 passed to CodeIgniter\Cache\Handlers\RedisHandler::__construct() must be of the type array, object given, called in ...

fixed it
Ci3 can be, I have been accustomed to write, because you can write functions, $where can be as a parameter by value, if there is no $where you can pass null, now to determine $where exists, is not very convenient

CI3 like this

public function getSort($db, $where = null, $order = 'id desc') {
        return $this->db->select('sort')->order_by($order)->where($where)->get()->result();
}
but now, CI4 like this

public function getSort($db, $where = null, $order = 'id desc') {
        $this->db->table($db)->select('sort')->orderBy($order);

        if ($where !== null) {
            $this->db->where($where);
        }

        return $this->db->get()->getResult();
}
This way, it doesn't get smooth!

So fixed it
@jinmarcus jinmarcus changed the title fix three pr fix redis + where + pageError Oct 24, 2017
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

where one must be removed. Then I could accept the other two.

@@ -574,8 +574,13 @@ public function join($table, $cond, $type = '', $escape = null)
*
* @return BaseBuilder
*/
public function where($key, $value = null, $escape = null)
public function where($key = null, $value = null, $escape = null)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry. Can't accept this one. $key is always required and it should fail loudly when the developer forgets to add it.

Copy link
Author

Choose a reason for hiding this comment

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

ok,Only follow this rule

This was referenced Oct 24, 2017
@lonnieezell
Copy link
Member

I wasn't thinking on your previous mention about where's and null. I don't think it's right to allow that in this case as it could be a bit dangerous and hard to track down during debugging. Sorry about that.

@lonnieezell
Copy link
Member

@jinmarcus Are you going to be able to revise this PR by removing the one fix so I can merge the others?

@jim-parry
Copy link
Contributor

Being picky, but we expect commits to be GPG-signed :)

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.

3 participants