Bugfix for #473. #476

Merged
merged 1 commit into from Mar 18, 2014

Conversation

Projects
None yet
6 participants
Owner

marcj commented Nov 4, 2013

Waww, that was more work than I thought. :D
While coding this I came unfortunately over a couple of other bugs. We need really some more closer look over the source code and need to make through some test-cases. We should not press out new Alpha versions until that.

TODO

  • Remove debug stuff/clean up some shit
  • Write documentation (propelorm/propelorm.github.com#278)
  • Write full list of bug fixes
  • Write more tests for the more complex n:n relation tables.
Owner

marcj commented Nov 5, 2013

  • Fixed bug: crossTable with at least one FK with multiple local columns
  • Fixed bug: crossTable with more than two FKs
  • Fixed bug: crossTable with two PK-FK and one PK-nonFK
  • Fixed bug: crossTable: setRelations(Collection), removeRelation($rel), save() doesn’t remove $rel.
  • Added Partial-checking also to CrossFK getter.
  • Fixed bug: wrong "if ($obj->isNew())” check in getRelation() methods.
  • Improved performance of hashCode.
  • Improved ObjectCollection checks
  • Modified clearAllReferences as in PHP 5.4 there's no workaround like that necessary anymore.
  • Fixed internal object relation reference tracking, especially unlinking back-references.
Owner

marcj commented Nov 5, 2013

Ok ladies, it's ready for the first review.

laugh

Member

jaugustin commented Nov 5, 2013

@marcj if it's to much of work we could keep it simple only allow isCrossRef on real m:n middle table and throw an exception if it's not the case.

There will be hundred's of cases that you can't even imagine like what append when versionnable, or inheritance behavior are added :)

Now I work on a simple case with a truly m:n but even that it doesn't work well (Propel1) objects are not saved as expected :(

Owner

marcj commented Nov 5, 2013

I'm very optimistic that we can achieve that 👍

Indeed, it's complicated, but however, that's the reason why the test for it is that big https://github.com/propelorm/Propel2/pull/476/files#diff-3b000e5180449f7effd37bb8eaa3b978R1 :D

tests/Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationTest.php
+use Propel\Tests\Helpers\PlatformDatabaseBuildTimeBase;
+
+/**
+ * This test proves the bug described in https://github.com/propelorm/Propel/issues/617.
@marcj

marcj Nov 5, 2013

Owner

lol, cop&paste issue

@marcj marcj referenced this pull request in propelorm/Propel Nov 5, 2013

Open

isCrossRef bug for ScheduledForDeletion collection #518

Owner

marcj commented Nov 10, 2013

Ok, now only the documentation is on the todo list. Perhaps someone can do a review?

Owner

marcj commented Nov 11, 2013

Mh, looks like travis found some bugs. Didn't have those on my local machine. Will check that in some days. SO all-clear, no codereview yet 👯

@marcj marcj referenced this pull request in propelorm/PropelBundle Dec 16, 2013

Closed

Foreign key order is important #262

Owner

marcj commented Jan 12, 2014

Ready for codereview. I've added now the fix for #477 as well as otherwise the CI won't be green.

src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
+ //$joinedTableObjectBuilder = $this->getNewObjectBuilder($fk->getTable());
+ //$className = $joinedTableObjectBuilder->getObjectClassName();
+
+ //$className = $this->getNewObjectBuilder($fk->getForeignTable())->getObjectClassname();
@marcj

marcj Jan 12, 2014

Owner

remove uncommented code

Owner

marcj commented Feb 6, 2014

Guys?

@marcj marcj closed this Feb 21, 2014

Member

staabm commented Feb 21, 2014

@marcj closed by mistake? The PR is very big, so review is rather hard..

Owner

marcj commented Feb 21, 2014

no mistake. it's ok to me when nobody finds the time to do a review.

@willdurand willdurand reopened this Feb 25, 2014

src/Propel/Generator/Builder/Om/ObjectBuilder.php
+ }
+";
+
+ //foreach ($queryClassNames as $queryClassName) {
@mpscholten

mpscholten Feb 25, 2014

Member

Looks like death code

src/Propel/Generator/Model/CrossForeignKeys.php
+ * @param ForeignKey $foreignKey
+ * @param Table $crossTable
+ */
+ function __construct(ForeignKey $foreignKey, Table $crossTable)
@mpscholten

mpscholten Feb 25, 2014

Member

I think this should be public to be consistent with all the other constructors.

src/Propel/Generator/Model/CrossForeignKeys.php
+ * @param ForeignKey $fk
+ * @return bool
+ */
+ public function isAtLeastOneLocalPrimaryIsNotCovered(ForeignKey $fk)
@mpscholten

mpscholten Feb 25, 2014

Member

Sorry if I'm wrong but shouldn't isAtLeastOneLocalPrimaryIsNotCovered be isAtLeastOneLocalPrimaryKeyNotCovered (without the second is, and with Key)?

@marcj

marcj Mar 13, 2014

Owner

you're right

src/Propel/Generator/Model/ForeignKey.php
+ */
+ public function hasLocalColumn(Column $column)
+ {
+ foreach ($this->getLocalColumnObjects() as $localColumn) {
@mpscholten

mpscholten Feb 25, 2014

Member

I think this can be replaced with return in_array($localColumn, $this->getLocalColumnObjects(), true)

src/Propel/Generator/Builder/Om/ObjectBuilder.php
+ if (\$validPk) {
+ return crc32(json_encode(\$this->getPrimaryKey(), JSON_UNESCAPED_UNICODE));
+ } else if (\$validPrimaryKeyFKs) {
+ return crc32(json_encode(\$primaryKeyFKs));
@mpscholten

mpscholten Feb 25, 2014

Member

Is there a reason you are using JSON_UNESCAPED_UNICODE above but not here in this line?

@marcj

marcj Mar 13, 2014

Owner

No, I'm gonna fix it

src/Propel/Generator/Model/Diff/DatabaseComparator.php
@@ -152,21 +152,6 @@ public function compareTables($caseInsensitive = false)
}
}
- $renamed = [];
@mpscholten

mpscholten Feb 25, 2014

Member

Whats the reason behind removing table renamings? It's a very basic feature of database migrations.

@marcj

marcj Feb 27, 2014

Owner

marcj commented 2 months ago

I've added now the fix for #477 as well as otherwise the CI won't be green.

@mpscholten

mpscholten Feb 27, 2014

Member

Thanks for the hint

Member

mpscholten commented Feb 25, 2014

Hey @marcj, I tried to review this, but it's hard to understand all the bug fixes. Maybe you can split this pull request so it's easier to review. Otherwise there's still lots of death code in this pull request. However most of it looks good 👍

Owner

marcj commented Feb 27, 2014

Good review, thanks. I unfortunately can't split this pull requests. It fixes too many things that are highly related to each other.

Member

mpscholten commented Feb 27, 2014

I unfortunately can't split this pull requests. It fixes too many things that are highly related to each other.

Ok, but due the fact that tests are passing and you're a propel core dev, I see this as mergable.

src/Propel/Generator/Builder/Om/ObjectBuilder.php
- $script .= "
+// $relCol = $this->getCrossFKsPhpNameAffix($crossFKs, true);
+// $collName = $this->getCrossFKsVarName($crossFKs);
+
src/Propel/Generator/Builder/Om/ObjectBuilder.php
- $tblFK = $refFK->getTable();
- $foreignObjectName = '$' . $tblFK->getStudlyPhpName();
+
+ $selfRelationName = $this->getFKPhpNameAffix($crossFKs->getIncomingForeignKey(), $plural = false);
@marcj

marcj Mar 13, 2014

Owner

indentation

src/Propel/Generator/Builder/Om/ObjectBuilder.php
+// $foreign = $map['foreign'];
+// $script .= "
+// {$foreignObjectName}->set{$local->getPhpName()}(\${$lowerRelatedObjectClassName}->get{$foreign->getPhpName()}());";
+// }
src/Propel/Generator/Builder/Om/ObjectBuilder.php
if (null === \$this->{$M2MScheduledForDeletion}) {
\$this->{$M2MScheduledForDeletion} = clone \$this->{$collName};
\$this->{$M2MScheduledForDeletion}->clear();
}
- \$this->{$M2MScheduledForDeletion}[] = {$crossObjectName};
+ \$this->{$M2MScheduledForDeletion}->push({$shortSignature});
+ //ar_dump(\$this->{$M2MScheduledForDeletion});
src/Propel/Generator/Util/QuickBuilder.php
+
+// $database = $builder->getDatabase();
+// $database->setSchema('migration');
+// $database->setPlatform($this->database->getPlatform());
...opel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationSimpleTest.php
+ * ####################################
+ * Special: addFriend, addFriend, removeFriend
+ *
+ * @group test
.travis.yml
@@ -13,7 +13,7 @@ env:
before_script:
# Composer
- wget http://getcomposer.org/composer.phar
- - php composer.phar install --prefer-source
+ - php composer.phar install --prefer-dist
@mpscholten

mpscholten Mar 13, 2014

Member

What's the reason behind this? On travis the composer updates should be loaded from source otherwise you will end up with random failing builds because github's api limit is reached. See composer/composer#1314

@marcj

marcj Mar 17, 2014

Owner

reverted

Owner

willdurand commented Mar 16, 2014

Let's rebase and merge this as is. It's ok to me.

@willdurand willdurand modified the milestones: alpha-3, 2.0.0 Mar 16, 2014

@marcj marcj referenced this pull request in propelorm/propelorm.github.com Mar 17, 2014

Closed

Docu for propelorm/Propel2#476 #278

Owner

marcj commented Mar 17, 2014

I've seen that after the rebase the hhvm build hangs on a couple of tests for some time. I guess it hangs on my new introduced tests, but not sure yet. Hm.

+
+ $script .= ";\n";
+
+ /** @var $primaryKeyFKs ForeignKey[] */
@gharlan

gharlan Mar 17, 2014

Contributor

imo it should be @var ForeignKey[] $primaryKeyFKs (first the type, then the variable)..

src/Propel/Runtime/Collection/ObjectCombinationCollection.php
+ $hashes = [];
+ $isActiveRecord = [];
+ foreach (func_get_args() as $pos => $obj) {
+ if (is_object($obj) && $obj instanceof ActiveRecordInterface) {
@gharlan

gharlan Mar 17, 2014

Contributor

is_object is needless

+ list($names) = $this->getCrossFKInformation($crossFKs);
+ $script .= "
+ /**
+ * @var ObjectCombinationCollection Cross CombinationCollection to store aggregation of $names combinations.
@gharlan

gharlan Mar 17, 2014

Contributor

you should add the classname of stored objects, like @var ObjectCombinationCollection|{$className}[] ...

@marcj

marcj Mar 17, 2014

Owner

No, since it's not based on only one class.

+ $script .= "
+ /**
+ * An array of objects scheduled for deletion.
+ * @var ObjectCollection
@gharlan

gharlan Mar 17, 2014

Contributor

same here

+
+ $script .= "
+ /**
+ * @var {$className}[] Cross Collection to store aggregation of $className objects.
@gharlan

gharlan Mar 17, 2014

Contributor

Please add ObjectCollection to var type, like @var ObjectCollection|{$className}[] ....

src/Propel/Generator/Builder/Om/ObjectBuilder.php
+ $combinationIdx = 0;
+ foreach ($crossFKs->getCrossForeignKeys() as $crossFK) {
+ $script .= "
+ //\$combination[$combinationIdx] = {$crossFK->getForeignTable()->getPhpName()} ({$crossFK->getName()})";
@gharlan

gharlan Mar 17, 2014

Contributor

are these commented code lines like this intended?

src/Propel/Generator/Builder/Om/ObjectBuilder.php
+ * @param array $phpDoc
+ */
+ protected function extractCrossInformation(CrossForeignKeys $crossFKs, ForeignKey $crossFKToIgnore = null,
+ &$signature, &$shortSignature, &$normalizedShortSignature, &$phpDoc)
@gharlan

gharlan Mar 17, 2014

Contributor

please use PSR-2 coding style..

Owner

marcj commented Mar 17, 2014

Interesting. hhvm hangs at the DROP TABLE IF EXISTS query for almost a minute. 👎

Member

staabm commented Mar 18, 2014

@marcj hhvm will cut a new this week, lets see how long it takes to get to the travis VMs... maybe this fixed one of the underlying issues.

Fixed relation handling. Fixed #473, #518, #262.
Fixed bug: crossTable with at least one FK with multiple local columns
Fixed bug: crossTable with more than two FKs
Fixed bug: crossTable with two PK-FK and one PK-nonFK
Fixed bug: crossTable: setRelations(Collection),
removeRelation($rel), save() doesn’t remove $rel.
Added Partial-checking also to CrossFK getter.
Fixed bug: wrong "if ($obj->isNew())” check in getRelation() methods.
Improved performance of hashCode.
Improved ObjectCollection checks
Modified clearAllReferences as in PHP 5.4 there's no workaround like
that necessary anymore.
Fixed internal object relation reference tracking, especially
unlinking back-references.

Added also the table rename fix for #477, so we do not rename tables
anymore. This was necessary because otherwise with this new incoming
tests for relations the test suite won't be green.
Owner

marcj commented Mar 18, 2014

Ok, I've tried to debug this, but no chance. hhvm hangs and waits at readv from MySQL untill a timeout kills it. I'm going to land it and we try to investigate it further with new hhvm releases.

marcj added a commit that referenced this pull request Mar 18, 2014

@marcj marcj merged commit 06a4665 into propelorm:master Mar 18, 2014

1 check passed

default The Travis CI build passed
Details
Owner

marcj commented Mar 18, 2014

Bam. 🚢ed

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