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

[2.1.x][Bug] PHQL subquery cant bind params for it- only for "array" type #11605

Closed
borisdelev opened this Issue Mar 29, 2016 · 28 comments

Comments

Projects
None yet
9 participants
@borisdelev
Contributor

borisdelev commented Mar 29, 2016

Hi there,
I think i found a issue with sub-queries. Let get look that example:

  $prepare = [
    'categories' => [1, 2, 3, 4, 5, 6, 10, 11]
  ];

  $phql = "SELECT * FROM Models\Teachers WHERE id IN (
    SELECT teacher_id FROM Models\CategoriesTeachers WHERE category_id IN ({categories:array})
  )";

  $query = $this->modelsManager->createQuery($phql);
  $elements = $query->execute($prepare);

So... Phalcon returns an error for me: "Bind value is required for array type placeholder: categories" - but it is prepared. I try this in older versions of phalcon- still make an error. If i am doing something wrong, please, correct me.
Just one more detail- if i use not array as param, but string or int category_id = {category:int} there is no problem- issue is only with array type.

Thank you!

@borisdelev borisdelev changed the title from 2.1RC1 subquery cant bind params for it (array) to 2.1RC1 subquery cant bind params for it (only for type array) Mar 29, 2016

@borisdelev borisdelev changed the title from 2.1RC1 subquery cant bind params for it (only for type array) to [2.1.x][Issue] subquery cant bind params for it (only for type array) Mar 29, 2016

@borisdelev borisdelev changed the title from [2.1.x][Issue] subquery cant bind params for it (only for type array) to [2.1.x][Bug] subquery cant bind params for it (only for type array) Mar 29, 2016

@borisdelev borisdelev changed the title from [2.1.x][Bug] subquery cant bind params for it (only for type array) to [2.1.x][Bug] PHQL subquery cant bind params for it (only for type array) Mar 29, 2016

@borisdelev borisdelev changed the title from [2.1.x][Bug] PHQL subquery cant bind params for it (only for type array) to [2.1.x][Bug] PHQL subquery cant bind params for it- only for "array" type Mar 29, 2016

@borisdelev

This comment has been minimized.

Contributor

borisdelev commented Apr 10, 2016

Hi,
No one respond to me :/ however, i find that this did not work in models relations:

        $this->hasMany('id', 'Models\ArticleMultimedia', 'article_id', [
            'alias' => 'MultimediaVideos',
            'params' => [
                'conditions' => 'type IN ({type:array})',
                'bind' => ['type' => ['video', 'youtube']],
                'order' => 'position ASC'
            ]
        ]);

With conditions like string it is working.
I am pretty sure this is happened in other situations too.

Thanks. Good luck.

@Jurigag

This comment has been minimized.

Member

Jurigag commented Apr 10, 2016

Is it working just with:

ArticleMultimedia::find() ?

Are you sure you are on 2.1.x version ?

@borisdelev

This comment has been minimized.

Contributor

borisdelev commented Apr 11, 2016

Hi there and thank you for comment. I check now, so:

  • phpinfo : Phalcon version is : 2.1.0r on PHP 5.6.3.
  • ArticleMultimedia::find() - work with all cases (test situations):
        $check = \Models\Multimedia::find([
            'conditions' => 'type={type:str} AND (subtype IN ({subtype:array})) AND parent_id IN (SELECT \Models\Articles.id FROM \Models\Articles WHERE \Models\Articles.id IN ({articles:array}))',
            'bind' => [
                'type' => 'Models\Articles',
                'subtype' => ['youtube', 'video'],
                'articles' => [1,2,3,4,5,6,7]
            ]
        ]);

Still next code did not work - get exception for a missing bind value:

        $prepare = [
          'articles' => [1, 2, 3, 4, 5, 6, 10, 11]
        ];

        $phql = "SELECT * FROM Models\Multimedia WHERE parent_id IN (
          SELECT Models\Articles.id FROM Models\Articles WHERE Models\Articles.id IN ({articles:array})
        )";

        $query = $this->modelsManager->createQuery($phql);
        $elements = $query->execute($prepare);

And that is it :/ Thank you. Wish u luck!

@borisdelev

This comment has been minimized.

Contributor

borisdelev commented Apr 20, 2016

Did someone check that?

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Apr 20, 2016

I'll try to sort out as soon as I can

@borisdelev

This comment has been minimized.

Contributor

borisdelev commented Apr 21, 2016

Thank you. May the force be with u! :) always :)

@dschissler

This comment has been minimized.

Contributor

dschissler commented May 11, 2016

This is affecting me as well. I'm dead in the water.

CREATE TABLE `hats` (
  `id` int(11) NOT NULL,
  `name` varchar(25) COLLATE utf8_unicode_ci NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

INSERT INTO `hats` (`id`, `name`) VALUES
(1, 'sombrero'),
(2, 'fedora'),
(3, 'tophat'),
(4, 'baseball'),
(5, 'panama');

ALTER TABLE `hats`
  ADD PRIMARY KEY (`id`);

ALTER TABLE `hats`
MODIFY `id` int(11) NOT NULL AUTO_INCREMENT, AUTO_INCREMENT=6;

CREATE TABLE `colors` (
  `id` int(11) NOT NULL,
  `name` varchar(25) COLLATE utf8_unicode_ci NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

INSERT INTO `colors` (`id`, `name`) VALUES
(1, 'green'),
(2, 'blue'),
(3, 'red'),
(4, 'black'),
(5, 'white');

ALTER TABLE `colors`
  ADD PRIMARY KEY (`id`);

ALTER TABLE `colors`
  MODIFY `id` int(11) NOT NULL AUTO_INCREMENT, AUTO_INCREMENT=6;

CREATE TABLE `hats_colors` (
  `id` int(11) NOT NULL,
  `hats_id` int(11) NOT NULL,
  `colors_id` int(11) NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

INSERT INTO `hats_colors` (`id`, `hats_id`, `colors_id`) VALUES
(1, 1, 3),
(2, 1, 4),
(3, 2, 4),
(4, 3, 5),
(5, 4, 2);

ALTER TABLE `hats_colors`
  ADD PRIMARY KEY (`id`);

ALTER TABLE `hats_colors`
  MODIFY `id` int(11) NOT NULL AUTO_INCREMENT, AUTO_INCREMENT=6;
<?php

class Hats extends Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->hasManyToMany(
            "id",
            "HatsColors",
            "hats_id",
            "colors_id",
            "Colors",
            "id",
            [
                'alias' => 'colors'
            ]
        );
    }
}
<?php

class Colors extends Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->hasManyToMany(
            "id",
            "HatsColors",
            "colors_id",
            "hats_id",
            "Hats",
            "id",
            [
                'alias' => 'hats'
            ]
        );
    }
}
<?php

class HatsColors extends Phalcon\Mvc\Model
{
    public function initialize()
    {
    }
}
        $phql = "SELECT Hats.* FROM Hats JOIN Colors WHERE Colors.name IN ({colors:array})";
        $query = new Phalcon\Mvc\Model\Query($phql, $this->getDI());
        $this->response->appendContent('<p>' . $query->getSql()['sql'] . '</p>');

        $hats = $query->execute([
            'colors' => ['red', 'green', 'black']
        ]);
Bind value is required for array type placeholder: colors

edit UPDATE: I've been able to get this to work by binding the variables first before the execute:

        $query->setBindParams([
            'colors' => ['red', 'green', 'black']
        ]);
        $hats = $query->execute();
@dschissler

This comment has been minimized.

Contributor

dschissler commented May 12, 2016

I discovered some weird undocumented behaviour.

$phql = "SELECT * FROM Hats WHERE name IN ({names:array})";
$query = new Phalcon\Mvc\Model\Query($phql, $this->getDI());

// This line would fail because it can't expand the array into placeholders
// because it doesn't know how many bind values there will be.
// error_log($query->getSql()['sql']);

// You must bind before calling getSql()
$query->setBindParams([
    'names' => ['tophat', 'baseball', 'panama']
]);

// So binding with execute won't work if you want to view the generated SQL
// before the query is executed.
$hats = $query->execute([
    'names' => ['tophat', 'baseball', 'panama']
]);

So this makes binding with execute wonky when using arrays. This likely wasn't considered when it was designed because the automatic array binding came much later.

@andresgutierrez Is there any way to make this more clear?

@Jurigag

This comment has been minimized.

Member

Jurigag commented May 12, 2016

Im just guessing it's working for bindParams and don't for execute. Why ? Beacause i guess execute just pass params to PDO execute. And bindParams are handled in phql by phalcon maybe ?

@dschissler

This comment has been minimized.

Contributor

dschissler commented May 12, 2016

I thought that at first as well when I discovered this new issue but I also found the original bug issue to be completely broken as the issue title suggests. The problem was that it presented a sort of Heisenbug because trying to print out the SQL with getSql() causes it to break.

@Jurigag

This comment has been minimized.

Member

Jurigag commented May 12, 2016

Im guessing it only works maybe for setBindParams(conditions) etc ? And if we use getSql() it doesn't know to handle parameter as array ? I don't know why it breaks if it's not a case.

@dschissler

This comment has been minimized.

Contributor

dschissler commented May 12, 2016

In my later example with without a join you can call getSql() after binding. The issue is that if you don't bind first then it doesn't/wouldn't know how many placeholders to add in since that is what it ultimately does since that is what PDO understands.

This makes me think that placing bind values directly on execute should be removed as an option in Phalcon since it doesn't function properly in all cases.

[edit] I meant without a join.

@Jurigag

This comment has been minimized.

Member

Jurigag commented May 12, 2016

No no, it should stayed. Sometimes you want to bind value to having, what in this case then ?

@dschissler

This comment has been minimized.

Contributor

dschissler commented May 12, 2016

Then you use $query->setBindParams any number of times as desired before calling execute with no parameters.

The issue with the current API is lets say that someone is binding all at once on execute and they are also binding an array then they are pretty much unable to inspect their SQL with getSQL() before the query fires off and if that query caused an error then they wouldn't know what was being generated. So this would make it pretty difficult for people to figure out how to correctly set it up. Just a lot of frustrating time wasted.

@dschissler

This comment has been minimized.

Contributor

dschissler commented May 12, 2016

Also to make it so that the situation can't be fouled up it should be impossible to bind more parameters after calling getSql(). It would only be relevant in the array binding case but it would prevent difficult to understand bugs.

@dschissler

This comment has been minimized.

Contributor

dschissler commented Jun 10, 2016

@sergeyklay Please add this to the 2.1 milestone as this is very serious for those affected by it and its a regression of relied upon behaviour.

@sergeyklay sergeyklay added this to the 2.1.0 milestone Jun 10, 2016

@andresgutierrez andresgutierrez modified the milestones: 2.1.1, 2.1.0 Jun 27, 2016

@sergeyklay

This comment has been minimized.

Member

sergeyklay commented Aug 13, 2016

@dschissler

its a regression of relied upon behaviour

Could you please explain a bit more?

@dschissler

This comment has been minimized.

Contributor

dschissler commented Aug 14, 2016

Some time ago in a 2.0.x Phalcon blog post it was announced that queries can now bind to arrays using the syntax that I used. Then sometime in the 2.1.x development tree it broke.

I think that it should be clear what needs to be fixed here. I can try it again with the latest code since it has been a while. After this issue is fixed I will then create a low priority issue to work out the timings of when $query->getSql()['sql'] can be used.

@dschissler

This comment has been minimized.

Contributor

dschissler commented Aug 14, 2016

@sergeyklay My Hats test sets it up nicely to fail. Perhaps Phalcon is missing a test for this array binding feature.

@sergeyklay sergeyklay self-assigned this Aug 14, 2016

@sergeyklay sergeyklay removed this from the 3.0.1 milestone Aug 22, 2016

@sergeyklay sergeyklay modified the milestones: 3.0.4, 3.0.3 Dec 24, 2016

@sergeyklay sergeyklay modified the milestones: 3.0.4, 3.1.x Feb 19, 2017

@FaimMedia

This comment has been minimized.

FaimMedia commented Aug 9, 2017

Got the same error message, not really in a subquery, but in a SUM of a HAVING clause:

SELECT * FROM `test`
GROUP BY `rule_id`
HAVING SUM(`field` IN ({value:array}) OR `field` IS NULL) >= 0

Running PHP 7.1.5 with Phalcon 3.1.2

@Jurigag

This comment has been minimized.

Member

Jurigag commented Aug 9, 2017

And how your binding array looks like?

@FaimMedia

This comment has been minimized.

FaimMedia commented Aug 11, 2017

Just some random id's, nothing special

@sergeyklay sergeyklay modified the milestones: 3.2.x, 3.3.x Oct 14, 2017

@CameronHall

This comment has been minimized.

Contributor

CameronHall commented Dec 18, 2017

I don't know if what I'm adding is of value, but I thought it could be useful. The generated SQL looks like this when doing getRelated;

IN (:hosts0)

The tokens are the issue as the values we intend to bind are being handled correctly.

@sergeyklay sergeyklay modified the milestones: 3.3.x, 3.4.x Mar 24, 2018

@sergeyklay sergeyklay removed this from the 3.4.x milestone Jun 1, 2018

@stale

This comment has been minimized.

stale bot commented Aug 30, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale label Aug 30, 2018

@CameronHall

This comment has been minimized.

Contributor

CameronHall commented Aug 30, 2018

This is still an issue.

@stale stale bot removed the stale label Aug 30, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Sep 2, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Oct 5, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Oct 5, 2018

Fixes phalcon#11605: Parameters are stored in _bindParams when callin…
…g execute

Updated CHANGELOG-3.4.md

Fixed failing tests caused by PR#13100

niden added a commit that referenced this issue Oct 5, 2018

Merge pull request #13480 from CameronHall/bugfix/subquery-binded-params
Fixes #11605: Parameters are stored in _bindParams when calling query execute
@CameronHall

This comment has been minimized.

Contributor

CameronHall commented Oct 5, 2018

Should be resolved as of #13480

@niden

This comment has been minimized.

Member

niden commented Oct 5, 2018

Thank you again Cameron

@niden niden closed this Oct 5, 2018

@sergeyklay sergeyklay added this to the 3.4.2 milestone Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment