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

criteria: fix issue #2157 and added tests #2208

Merged
merged 10 commits into from
Mar 31, 2014
Merged

Conversation

maxgalbu
Copy link
Contributor

See the discussion here:
http://forum.phalconphp.com/discussion/1479/multiple-joins-in-oo-notation-fail

From what I saw, the problem was that the current_joins was merged with the join array instead of appended, so it resulted in something like this:

array(
    //first join
    0 => array(
        'type' => "LEFT",
        'conditions' => "...",
    ),

    //all the following joins
    'type' => "LEFT",
    'conditions' => "...",
)

instead of

array(
    //first join
    0 => array(
        'type' => "LEFT",
        'conditions' => "...",
    ),

    //all the following joins
    1 => array(
        'type' => "LEFT",
        'conditions' => "...",
    ),
)

This is my first attempt at editing phalcon code, I have some experience with c code but none with php extension. I didn't tried but i guessed I couldn't edit current_joins directly, so phalcon_array_append is out of the question. I don't know how to copy a zval, so to avoid editing too much code I simpy added an array of array that can be merged with current_joins or copy-written on merged_joins.

@phalcon
Copy link
Collaborator

phalcon commented Mar 21, 2014

Hi, thanks for trying to fix this, could you please submit this to 1.3.2?

phalcon pushed a commit that referenced this pull request Mar 31, 2014
criteria: fix issue #2157 and added tests
@phalcon phalcon merged commit 7b90cff into phalcon:1.3.1 Mar 31, 2014
@phalcon
Copy link
Collaborator

phalcon commented Mar 31, 2014

Thanks

@maxgalbu
Copy link
Contributor Author

Wait, you merged my 1.3.2 based branch on your 1.3.1 branch so some commits that were done on 1.3.2 slipped onto your 1.3.1 branch, is that what you wanted?
Sorry for that, I force-updated my branch instead of creating a new one for 1.3.2 :(

@phalcon
Copy link
Collaborator

phalcon commented Mar 31, 2014

No, I thought the old PR was closed and merged it :'(

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.

6 participants