Skip to content

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 :)

// 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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants