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 issue #1388: broken model-only relationships #1389

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Alexander-Bliznyuk
Copy link
Contributor

No description provided.

$noExceptionThrown = false;
}

$this->assertTrue($noExceptionThrown);
Copy link
Member

@marcj marcj Jun 12, 2017

Choose a reason for hiding this comment

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

When an exception is thrown this test is automatically marked as failed, so no need for that assertTrue :)

@@ -791,7 +791,7 @@ public function setupReferrers($throwErrors = false)
$foreignPrimaryKeys = $foreignTable->getPrimaryKey();
// check all keys are referenced in foreign key
foreach ($foreignPrimaryKeys as $foreignPrimaryKey) {
if (!$foreignPrimaryKey->hasReferrer($foreignKey) && $throwErrors) {
if (!$foreignPrimaryKey->hasReferrer($foreignKey) && !$foreignKey->isSkipSql() && $throwErrors) {
Copy link
Member

Choose a reason for hiding this comment

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

well, generating SQL has nothing to do with checking for valid foreign keys. So this check is a misuse of the skip-sql attribute to allow models generation with invalid foreign keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Propel used to allow flexible models tailored for application's needs. This checking for foreign keys merely ties hands. For example, if referent table has composite key, it appears to be impossible to join it by a column outside the key.
Please also consider fully justified and common scenario: i18n tables having primary key(id, locale). Of course in many cases other tables would refer to i18n tables only by id. Another similar common scenario is versioning of entities.
Maybe it's ok to be strict and thus guarantee scheme validity between different RDBMS and to guide amateurs.
But model only relationships feature shouldn't be broken. It doesn't work now as described in docs and my pull request fixes this regression.

@dereuromark dereuromark added the Bug label Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants