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.

Sign up for GitHub

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

Jump to bottom

Fix issue #1388: broken model-only relationships #1389

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

Fix issue #1388: broken model-only relationships #1389

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea629f8
Fix issue #1388: broken model-only relationships
Alexander-Bliznyuk Jun 12, 2017
3d61fef
simplified test
Alexander-Bliznyuk Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Propel/Generator/Model/Table.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

// foreign primary key is not being referenced in foreign key
throw new BuildException(sprintf(
'Table "%s" contains a foreign key to table "%s" but does not have a reference to foreign primary key "%s"',
Expand Down
58 changes: 58 additions & 0 deletions tests/Propel/Tests/Issues/Issue1388Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php
/**
* This file is part of the Propel package.
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @license MIT License
*/
namespace Propel\Tests\Issues;

use Propel\Generator\Util\QuickBuilder;
use Propel\Tests\TestCaseFixtures;

/**
* Test : Propel should allow marked to be skipped foreign key references on table with composite primary key
* @group model
*/
class Issue1388Test extends TestCaseFixtures
{
public function testSkippedIncompleteForeignReference()
{
$schema = <<<EOF
<?xml version="1.0" encoding="utf-8"?>
<database name="test" defaultIdMethod="native">
<table name="event">
<column name="id" type="integer" required="true" primaryKey="true" autoIncrement="true" />
<column name="name" type="varchar" size="50" required="true" />
<column name="organiser_type_id" type="integer" required="true" />
<!-- This FK is incomplete -->
<foreign-key foreignTable="organiser" skipSql="true">
<reference local="organiser_type_id" foreign="type_id" />
</foreign-key>
</table>

<table name="organiser">
<!-- Has composite PK -->
<column name="id" type="integer" required="true" primaryKey="true" autoIncrement="true" />
<column name="secondary" type="integer" required="true" primaryKey="true" />
<column name="type_id" type="integer" required="true" />
<column name="name" type="varchar" size="50" required="true" />
</table>
</database>
EOF;

$builder = new QuickBuilder();
$builder->setSchema($schema);

try {
$builder->buildClasses(null, true);
$noExceptionThrown = true;
} catch (\Exception $e) {
$noExceptionThrown = false;
}

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

@marcj marcj Jun 12, 2017 •
edited
Loading

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


}
}