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

func_get_args() call order fix for HHVM bug #1142

Merged
merged 1 commit into from
Sep 22, 2014
Merged

Conversation

BenjaminNolan
Copy link

Firstly, so no-one closes this as ENOTONMASTER, @Ocramius told me to open it directly against 2.4. :)

This PR fixes bugs in Doctrine\ORM\QueryBuilder where the values provided to method arguments to ::andWhere(), ::orWhere(), ::andHaving(), and ::orHaving() are overwritten by code within the methods in HHVM and PHP7.

After much fun and headscratching in both #doctrine and #hhvm on Freenode, we discovered that in the following code, $where = $this->getDqlPart('where'); overwrites the values stored internally for the $where parameter of the function which is returned by func_get_args():

public function orWhere($where)
{
    $where = $this->getDqlPart('where');
    $args  = func_get_args();
}

This means that instead of $args being set as expected to an array containing the string or object provided to the function, it actually contains the object returned by QueryBuilder::getDqlPart(), which causes all the problems you'd expect it to.

This PR simply swaps the order of the calls so that the function arguments are retrieved via func_get_args() before the $where variable is modified, fixing the problem.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. However did not open it on the "master"
branch. Our Git workflow requires all pull requests to go through "master" branch
and the release masters then merge them back into stable branches, if they are
bug fixes.

Please open the pull request again for the "master" branch and close
this one.

Nevertheless I have opened a Jira ticket for this Pull Request to track this
issue:

http://www.doctrine-project.org/jira/browse/DDC-3317

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius
Copy link
Member

Related: facebook/hhvm#3816

@Ocramius Ocramius self-assigned this Sep 22, 2014
@Ocramius
Copy link
Member

@TwoWholeWorms can you also check if this affects master in any way?

@BenjaminNolan
Copy link
Author

By the looks of it, it's already been fixed in all these functions in master (which I'd guess is the 2.5.*-dev tree), so I think the 2.4-only fix should suffice. I could back-port it further, but I highly doubt anyone is using anything earlier than 2.4 on HHVM. Actually, TBH, I highly doubt anyone is using 2.4 on HHVM at all, since it's, y'know… b0rked.

@Ocramius
Copy link
Member

Ok, fine with merging then. I'll also tag.

Ocramius added a commit that referenced this pull request Sep 22, 2014
func_get_args() call order fix for HHVM bug
@Ocramius Ocramius merged commit a50ae2c into doctrine:2.4 Sep 22, 2014
@till
Copy link

till commented Nov 3, 2014

@Ocramius @TwoWholeWorms — Doesn't look like this fixes much? A test case would have been nice.

greg0ire pushed a commit to sonata-project/SonataDoctrineORMAdminBundle that referenced this pull request Oct 1, 2017
core23 pushed a commit to sonata-project/SonataDoctrineORMAdminBundle that referenced this pull request Oct 2, 2017
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.

4 participants