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

Fixed virtual foreign key check #12068

Merged
merged 2 commits into from Aug 4, 2016
Merged

Fixed virtual foreign key check #12068

merged 2 commits into from Aug 4, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 1, 2016

Hi,

I am reopening PR #11975 for 3.0.x branch.

@ghost
Copy link
Author

ghost commented Aug 1, 2016

@sergeyklay I have returned the default value for validateWithNulls variable, but it is set also in the for loop now.

Wouldn't it be more friendly to set it only once in the for loop? It's not used anywhere the outside of the for loop I guess, so assigning the value twice might not be that readable.

@sergeyklay
Copy link
Contributor

Your PR does not make sense right now because the validateWithNulls has been set previously, before the loop

@ghost
Copy link
Author

ghost commented Aug 1, 2016

I do not understand. That was originally the problem, when it was set before the loop.

Because when you have a model with more than one virtual foreign key, first foreign key check leaves the validateWithNulls set and might be incorrect for the next for loop iteration.

That is why I removed the default value and put the assignment inside the for loop.

@Jurigag
Copy link
Contributor

Jurigag commented Aug 1, 2016

He just means if relation is null then if there is enabled notNullValidations it should pass.

@sergeyklay
Copy link
Contributor

Okay, could you please provide a small test which cover this case?

@ghost
Copy link
Author

ghost commented Aug 1, 2016

I'll definitely give it a try.

@sergeyklay
Copy link
Contributor

sergeyklay commented Aug 1, 2016

@crnix I have investigated the code more details. Yes. You're right. It be more correct to set it the for loop. But in any case your PR need to be tested.

@sergeyklay
Copy link
Contributor

Also could you please find the issue or open new one with script to reproduce the problem and refer this PR to the issue

@ghost
Copy link
Author

ghost commented Aug 2, 2016

OK, so here is the issue with steps to reproduce this #12071

@ghost
Copy link
Author

ghost commented Aug 3, 2016

Could you explain to me please or refer me to some description what is the difference between tests and unit-tests folders?

Also, where can I define the database tables for the test? Is it OK to add them to tests/_data/schemas/mysql/mysql.dump.sql?

@sergeyklay
Copy link
Contributor

sergeyklay commented Aug 3, 2016

Could you explain to me please or refer me to some description what is the difference between tests and unit-tests folders?

Use always the tests folder. In the future all tests will be located in the the tests folder.

Also, where can I define the database tables for the test? Is it OK to add them to tests/_data/schemas/mysql/mysql.dump.sql?

Yes. But don't forget to amend tables https://github.com/phalcon/cphalcon/blob/3.0.x/tests/unit/Db/Adapter/Pdo/MysqlTest.php

@@ -3,6 +3,7 @@
- Fixed `Phalcon\Mvc\Model\Manager::getRelationRecords` to correct using multi relation column [#12035](https://github.com/phalcon/cphalcon/issues/12035)
- Fixed `Phalcon\Acl\Resource`. Now it implements `Phalcon\Acl\ResourceInterface` [#11959](https://github.com/phalcon/cphalcon/issues/11959)
- Fixed `save` method for all cache backends. Now it updates the `_lastKey` property correctly [#12050](https://github.com/phalcon/cphalcon/issues/12050)
- Fixed virtual foreign key check when having multiple keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Refer to the issue please

@sergeyklay
Copy link
Contributor

@sergeyklay sergeyklay added this to the 3.0.1 milestone Aug 4, 2016
@sergeyklay sergeyklay merged commit 43d6121 into phalcon:3.0.x Aug 4, 2016
@sergeyklay
Copy link
Contributor

Thanks!

@ghost
Copy link
Author

ghost commented Aug 4, 2016

You are welcome

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.

2 participants