Removed PEER, Fixed #183, and more. #359

Closed
wants to merge 1 commit into
from

Projects

None yet

8 participants

Owner
marcj commented Apr 14, 2013
$ grep 'peer' * -rin | wc -l
       0
  • Removed all peer stuff. …
  • Adjusted documentation, .xml etc.
  • Abstracted connection handling, so we don't deal with PDOStatement directly anymore, but with DataFetcher, which hides the adapter specific stuff. (PDOStatement etc.)
  • Abstracted OM build process, so the platform can overwrite which OM Build class we use. (getBuilderClass())
  • Made populateObjects name-indexed compatible.

Fixed #183

@marcj marcj referenced this pull request Apr 14, 2013
Closed

Removing PEER classes #183

Owner
marcj commented Apr 15, 2013

The abstracted connection handling (DataFetcher) works basically like:
Screen Shot 2013-04-15 at 10 46 53 PM

@willdurand willdurand commented on an outdated diff Apr 15, 2013
src/Propel/Runtime/Connection/ConnectionWrapper.php
* @see http://php.net/manual/en/pdo.query.php for a description of the possible parameters.
*
* @return PDOStatement
*/
- public function query()
+ public function query($statement)
willdurand
willdurand Apr 15, 2013 Owner

Is it useful as you don't use it?

@willdurand willdurand commented on an outdated diff Apr 15, 2013
src/Propel/Runtime/Formatter/ArrayFormatter.php
{
$this->checkInit();
+ if ($dataFetcher) {
+ $this->setDataFetcher($dataFetcher);
+ } else {
+ $dataFetcher = $this->getDataFetcher();
+ }
+
+ $dataFetcher = $this->getDataFetcher();
willdurand
willdurand Apr 15, 2013 Owner

you should remove this line

@willdurand willdurand commented on an outdated diff Apr 15, 2013
...el/Tests/Runtime/Formatter/ArrayFormatterWithTest.php
@@ -452,6 +449,9 @@ public function testFindOneWithClassAndColumn()
$this->assertEquals('Grass', $book['AuthorName2'], 'ArrayFormatter correctly hydrates all as columns');
}
+ /**
+ * @group test
@willdurand willdurand commented on an outdated diff Apr 15, 2013
...el/Tests/Runtime/Formatter/ArrayFormatterWithTest.php
@@ -468,6 +468,7 @@ public function testFindPkWithOneToMany()
->joinWith('Review')
->findPk($pk, $con);
$reviews = $book['Reviews'];
+ //var_dump($book);
willdurand
willdurand Apr 15, 2013 Owner

should be removed

Owner
marcj commented Apr 15, 2013

I will squash it again, when the review is done.

@willdurand willdurand commented on an outdated diff Apr 15, 2013
.../Behavior/AggregateColumn/templates/objectCompute.php
{
- $stmt = $con->prepare('<?php echo $sql ?>');
-<?php foreach ($bindings as $key => $binding): ?>
- $stmt->bindValue(':p<?php echo $key ?>', $this->get<?php echo $binding ?>());
-<?php endforeach; ?>
+ $stmt = $con->prepare('<?=$sql?>');
+<?php foreach ($bindings as $key => $binding):?>
+ $stmt->bindValue(':p<?=$key?>', $this->get<?=$binding?>());
willdurand
willdurand Apr 15, 2013 Owner

should be 4 spaces

@willdurand willdurand commented on an outdated diff Apr 15, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
@@ -10,9 +10,13 @@
namespace Propel\Runtime\ActiveQuery;
+use Behat\Gherkin\Exception\Exception;
willdurand
willdurand Apr 15, 2013 Owner

Gherkin? oO

@willdurand willdurand commented on an outdated diff Apr 15, 2013
src/Propel/Runtime/Formatter/AbstractFormatter.php
@@ -153,6 +157,13 @@ protected function getCollection()
return $collection;
}
+ /**
+ *
willdurand
willdurand Apr 15, 2013 Owner

PHP doc is missing

@willdurand willdurand commented on the diff Apr 15, 2013
tests/Fixtures/etc/lob/tin_drum.txt
@@ -51,7 +51,7 @@ Taking advantage of the intermission, my grandmother tried to spit another potat
He made it to the lane; short and wide, he had barely disappeared into the lane, when the two others, long and thin, who had probably been searching the brickyard in the meantime, climbed over the horizon and came plodding through the mud, so long and thin, but not really skinny, that my grandmother missed her potato again; because it's not every day that you see this kind of thing, three full-grown men, though they hadn't grown in exactly the same directions, hopping around telegraph poles, nearly breaking the chimney off the brickworks, and then at intervals, first short and wide, then long and thin, but all with the same difficulty, picking up more and more mud on the soles of their boots, leaping through the field that Vincent had plowed two days before, and disappearing down the sunken lane.
-Then all three of them were gone and my grandmother ventured to spit another potato, which by this time was almost cold. She hastily blew the earth and ashes off the skin, popped the whole potato straight into her mouth. They must be from the brickworks, she thought if she thought anything, and she was still chewing with a circular motion when one of them jumped out of the lane, wild eyes over a black mustache, reached the fire in two jumps, stood before, behind, and beside the fire all at once, cursing, scared, not knowing which way to go, unable to turn back, for behind him Long and Thin were running down the lane. He hit his knees, the eyes in his head were like to pop out, and sweat poured from his forehead. Panting, his whole face atremble, he ventured to crawl closer, toward the soles of my grandmother's boots, peering up at her like a squat little animal. Heaving a great sigh, which made her stop chewing on her potato, my grandmother let her feet tilt over, stopped thinking about bricks and brickmakers, and lifted high her skirt, no, all four skirts, high enough so that Short and Wide, who was not from the brickworks, could crawl underneath. Gone was his black mustache; he didn't look like an animal any more, he was neither from Ramkau nor from Viereck, at any rate he had vanished with his fright, he had ceased to be wide or short but he took up room just the same, he forgot to pant or tremble and he had stopped hitting his knees; all was as still as on the first day of Creation or the last; a bit of wind hummed in the potato fire, the telegraph poles counted themselves in silence, the chimney of the brickworks stood at attention, and my grandmother smoothed down her uppermost skirt neatly and sensibly over the second one; she scarcely felt him under her fourth skirt, and her third skirt wasn't even aware that there was anything new and unusual next to her skin. Yes, unusual it was, but the top was nicely smoothed out and the second and third layers didn't know a thing; and so she scraped two or three potatoes out of the ashes, took four raw ones from the basket beneath her right elbow, pushed the raw spuds one after another into the hot ashes, covered them over with more ashes, and poked the fire till the smoke rose in clouds--what else could she have done?
+Then all three of them were gone and my grandmother ventured to spit another potato, which by this time was almost cold. She hastily blew the earth and ashes off the skin, popped the whole potato straight into her mouth. They must be from the brickworks, she thought if she thought anything, and she was still chewing with a circular motion when one of them jumped out of the lane, wild eyes over a black mustache, reached the fire in two jumps, stood before, behind, and beside the fire all at once, cursing, scared, not knowing which way to go, unable to turn back, for behind him Long and Thin were running down the lane. He hit his knees, the eyes in his head were like to pop out, and sweat poured from his forehead. Panting, his whole face atremble, he ventured to crawl closer, toward the soles of my grandmother's boots, pering up at her like a squat little animal. Heaving a great sigh, which made her stop chewing on her potato, my grandmother let her feet tilt over, stopped thinking about bricks and brickmakers, and lifted high her skirt, no, all four skirts, high enough so that Short and Wide, who was not from the brickworks, could crawl underneath. Gone was his black mustache; he didn't look like an animal any more, he was neither from Ramkau nor from Viereck, at any rate he had vanished with his fright, he had ceased to be wide or short but he took up room just the same, he forgot to pant or tremble and he had stopped hitting his knees; all was as still as on the first day of Creation or the last; a bit of wind hummed in the potato fire, the telegraph poles counted themselves in silence, the chimney of the brickworks stood at attention, and my grandmother smoothed down her uppermost skirt neatly and sensibly over the second one; she scarcely felt him under her fourth skirt, and her third skirt wasn't even aware that there was anything new and unusual next to her skin. Yes, unusual it was, but the top was nicely smoothed out and the second and third layers didn't know a thing; and so she scraped two or three potatoes out of the ashes, took four raw ones from the basket beneath her right elbow, pushed the raw spuds one after another into the hot ashes, covered them over with more ashes, and poked the fire till the smoke rose in clouds--what else could she have done?
willdurand
willdurand Apr 15, 2013 Owner

peering or pering?

marcj
marcj Apr 15, 2013 Owner

Who cares - it's test text? xD I want a zero in my 'peer grep' and this peer is annoying. :-p

Owner

I don't see any tests for the DataFetcher things.

Owner

@marcj could you add tests for the DataFetcher?

@staabm, @jaugustin did you/could you review the PR?

Owner
marcj commented Apr 17, 2013

Yepp, writing currently tests.

Owner
marcj commented Apr 18, 2013

Ok, I guess I've done everything mentioned. 👯

Member
staabm commented Apr 18, 2013

Will have another look today

@staabm staabm commented on the diff Apr 18, 2013
documentation/cookbook/runtime-introspection.markdown
{% highlight php %}
<?php
-$bookTable = BookPeer::getTableMap();
+$bookTable = BookTableMap::getTableMap();
staabm
staabm Apr 18, 2013 Member

in other places you use Map\BookTableMap?

marcj
marcj Apr 18, 2013 Owner

yes, saw this already. sucks a bit that we need here always Map\ at the moment. But I saw somewhere a ticket that mentioned a \FooTableMap that extends from \Map\FooTableMap, but haven't found it yet.

@staabm staabm commented on the diff Apr 18, 2013
resources/xsd/database.xsd
@@ -322,10 +322,10 @@
</xs:documentation>
</xs:annotation>
</xs:attribute>
- <xs:attribute name="peerName" type="php_class" use="optional">
+ <xs:attribute name="tableMapName" type="php_class" use="optional">
staabm
staabm Apr 18, 2013 Member

is php_class the correct type? php_class allows dots which are not allowed for class-constants?

marcj
marcj Apr 19, 2013 Owner

Yepp, is correct.

@staabm staabm commented on the diff Apr 18, 2013
.../Behavior/AggregateColumn/templates/objectCompute.php
@@ -1,17 +1,17 @@
/**
- * Computes the value of the aggregate column <?php echo $column->getName() ?>
+ * Computes the value of the aggregate column <?=$column->getName()?>
staabm
staabm Apr 18, 2013 Member

do we have a CS for spacing when using <?=XX?>?

willdurand
willdurand Apr 18, 2013 Owner

Well, I would like to have spaces but it's not a problem here.

@staabm staabm commented on the diff Apr 18, 2013
...r/NestedSet/NestedSetBehaviorQueryBuilderModifier.php
@@ -555,11 +550,11 @@ static public function deleteTree(" . ($useScope ? "\$scope = null, " : "") . "C
\$c = new Criteria($tableMapClassName::DATABASE_NAME);
\$c->add($objectClassName::SCOPE_COL, \$scope, Criteria::EQUAL);
- return $peerClassName::doDelete(\$c, \$con);";
+ return $tableMapClassName::doDelete(\$c, \$con);";
staabm
staabm Apr 18, 2013 Member

do we need to make sure that the namespaced TableMap class is imported?

marcj
marcj Apr 19, 2013 Owner

No need for that, it's already.

@staabm staabm commented on the diff Apr 18, 2013
.../Generator/Behavior/QueryCache/QueryCacheBehavior.php
@@ -48,8 +50,8 @@ public function queryAttributes($builder)
public function queryMethods($builder)
{
- $builder->declareClasses('\Propel\Runtime\Propel', '\Propel\Runtime\Util\BasePeer');
- $this->peerClassName = $builder->getPeerClassName();
+ $builder->declareClasses('\Propel\Runtime\Propel');
+ $this->tableClassName = $builder->getTableMapClassName();
staabm
staabm Apr 18, 2013 Member

same hee... is a declareClass required for the TableMap?

marcj
marcj Apr 19, 2013 Owner

see above.

@staabm staabm commented on the diff Apr 18, 2013
...ior/Sortable/SortableBehaviorQueryBuilderModifier.php
+/**
+ * Return an array of sortable objects ordered by position
+ *
+ * @param Criteria \$criteria optional criteria object
+ * @param string \$order sorting order, to be chosen between Criteria::ASC (default) and Criteria::DESC
+ * @param ConnectionInterface \$con optional connection
+ *
+ * @return array list of sortable objects
+ */
+static public function doSelectOrderByRank(Criteria \$criteria = null, \$order = Criteria::ASC, ConnectionInterface \$con = null)
+{
+ if (null === \$con) {
+ \$con = Propel::getServiceContainer()->getReadConnection({$this->tableMapClassName}::DATABASE_NAME);
+ }
+
+ if (null === \$criteria) {
staabm
staabm Apr 18, 2013 Member

someone told me that in PHP a typehinted parameter cannot be passed null?

see http://www.php.net/manual/en/language.oop5.typehinting.php

willdurand
willdurand Apr 18, 2013 Owner

that one is optional

staabm
staabm Apr 18, 2013 Member

sure, but the php-doc states this is not possible in php.. (I use this kind of things in java very often BTW)

::doSelectOrderByRank(null, ... will result in a fatal error

staabm
staabm Apr 18, 2013 Member

just tried it on PHP 5.3 on my local machine and this indeed results in an error (maybe it is nevertheless allowed in 5.4+)

staabm
staabm Apr 18, 2013 Member

ahh I got it... passing null here is allowed, because the param defaults to null. if it won't have this default it wouldn't work. http://artur.ejsmont.org/blog/content/php-typehints-causing-errors-when-null-gets-passed-in

https://gist.github.com/staabm/67132ac9f7c27492a58a

willdurand
willdurand Apr 18, 2013 Owner

yeah, that's why I said it was an optional argument.

staabm
staabm Apr 18, 2013 Member

-> so we have no problem here...

@staabm staabm and 3 others commented on an outdated diff Apr 18, 2013
src/Propel/Runtime/DataFetcher/DataFetcher.php
@@ -0,0 +1,134 @@
+<?php
+
+namespace Propel\Runtime\DataFetcher;
+
+/**
+ * Class DataFetcher
+ *
+ * This class is to iterate through resultSets like PDOStatement, MongoDBCollection etc.
+ *
+ * @package Propel\Runtime\Formatter
+ */
+abstract class DataFetcher implements \Iterator
staabm
staabm Apr 18, 2013 Member

since this class is meant to iterate through result sets (stated in the comment above), its name should maybe reflect that. DataIterator?

willdurand
willdurand Apr 18, 2013 Owner

As it's an abstract class, I would name it AbstractDataFetcher

willdurand
willdurand Apr 18, 2013 Owner

DataIterator makes sense BTW

havvg
havvg Apr 18, 2013 Member

As this class is not implementing the Iterator, it should make use of IteratorAggregate to open implementation of external/existing/php-internal iterators; or is there any reason for this, I don't see right now?

It should implement Countable, too. The count method already exists.

From an architecture point of view, there is a domain specific interface missing to reflect the API of this class. Currently, it's not exchangeable. This interface should then exchange Iterator or IteratorAggregate as mentioned above with Traversable.

marcj
marcj Apr 18, 2013 Owner

It's not only the purpose of this class to iterate through it, so I don't see a reason to rename it into DataIterator. But I give +1 for AbsractDataFetcher and for Countable. @havvg, good point with the interface, totally missed it in that huge diff - I will implement that as well. Thanks, good feedback guys 👍

@staabm staabm and 2 others commented on an outdated diff Apr 18, 2013
src/Propel/Runtime/DataFetcher/DataFetcher.php
@@ -0,0 +1,134 @@
+<?php
+
+namespace Propel\Runtime\DataFetcher;
+
+/**
+ * Class DataFetcher
+ *
+ * This class is to iterate through resultSets like PDOStatement, MongoDBCollection etc.
+ *
+ * @package Propel\Runtime\Formatter
willdurand
willdurand Apr 18, 2013 Owner

yes, we don't use this stuff anymore

marcj
marcj Apr 18, 2013 Owner

which stuff do you mean @willdurand ?

@staabm staabm commented on an outdated diff Apr 18, 2013
src/Propel/Runtime/DataFetcher/DataFetcher.php
+ * @var mixed
+ */
+ protected $dataObject;
+
+ /**
+ * @param $dataObject
+ */
+ public function __construct($dataObject)
+ {
+ $this->setDataObject($dataObject);
+ }
+
+ /**
+ * Sets the dataObject.
+ *
+ * @param $dataObject
@staabm staabm commented on an outdated diff Apr 18, 2013
src/Propel/Runtime/DataFetcher/DataFetcher.php
+/**
+ * Class DataFetcher
+ *
+ * This class is to iterate through resultSets like PDOStatement, MongoDBCollection etc.
+ *
+ * @package Propel\Runtime\Formatter
+ */
+abstract class DataFetcher implements \Iterator
+{
+ /**
+ * @var mixed
+ */
+ protected $dataObject;
+
+ /**
+ * @param $dataObject
Member
staabm commented Apr 18, 2013

will continue reviewing later on... have to do some different things now

@staabm staabm commented on the diff Apr 19, 2013
.../Propel/Tests/Runtime/Util/TableMapExceptionsTest.php
{
public function testDoSelect()
{
try {
$c = new Criteria();
$c->add(BookTableMap::ID, 12, ' BAD SQL');
- BookPeer::addSelectColumns($c);
- BasePeer::doSelect($c);
- } catch (RuntimeException $e) {
+ BookTableMap::addSelectColumns($c);
+ $c->doSelect();
staabm
staabm Apr 19, 2013 Member

not related to your changes, but shouldn't these tests $this->fail() as last step of the try block to ensure that a exception is thrown and catched at all?

In other words: this tests lead to false positives when no exception is thrown.

marcj
marcj Apr 19, 2013 Owner

No idea. Maybe you can create for this a ticket for further inspection.

@staabm staabm and 1 other commented on an outdated diff Apr 19, 2013
src/Propel/Runtime/DataFetcher/ArrayDataFetcher.php
+ {
+ return $this->indexType;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function count()
+ {
+ return null === $this->dataObject ? : count($this->dataObject);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function setIndexType($indexType)
staabm
staabm Apr 19, 2013 Member

setter is neither part of the interface nor the abstract base class (therefore inheritdoc will not work). but is it required at all?

marcj
marcj Apr 19, 2013 Owner

Only for ArrayDataFetcher useful.

staabm
staabm Apr 27, 2013 Member

Ok, then docs need to be updated

@staabm staabm commented on an outdated diff Apr 19, 2013
src/Propel/Runtime/Connection/ConnectionWrapper.php
@@ -408,6 +408,9 @@ public function exec($sql)
*
* Overrides PDO::query() to log queries when required
*
+ * @param string $statement The SQL statement to prepare and execute.
staabm
staabm Apr 19, 2013 Member

phpdoc for 2nd param

@staabm staabm commented on an outdated diff Apr 19, 2013
src/Propel/Runtime/Connection/PdoConnection.php
+ * {@inheritDoc}
+ */
+ public function getDataFetcher($data)
+ {
+ return new PDODataFetcher($data);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function getSingleDataFetcher($data)
+ {
+ return $this->getDataFetcher($data);
+ }
+
+ public function query($statement)
staabm
staabm Apr 19, 2013 Member

inheritDoc

@staabm staabm commented on an outdated diff Apr 19, 2013
src/Propel/Runtime/Connection/ConnectionWrapper.php
@@ -408,6 +408,9 @@ public function exec($sql)
*
* Overrides PDO::query() to log queries when required
*
+ * @param string $statement The SQL statement to prepare and execute.
+ * Data inside the query should be properly escaped.
+ *
* @see http://php.net/manual/en/pdo.query.php for a description of the possible parameters.
*
* @return PDOStatement
staabm
staabm Apr 19, 2013 Member

StatementInterface

@staabm staabm commented on the diff Apr 19, 2013
documentation/documentation/02-buildtime.markdown
PublisherQuery.php
{% endhighlight %}
For every table in the database, Propel creates 3 PHP classes:
* a _model_ class (e.g. `Book`), which represents a row in the database;
-* a _peer_ class (e.g. `BookPeer`), offering static constants and methods mostly for compatibility with previous Propel versions;
+* a _tablemap_ class (e.g. `Map\BookTableMap`), offering static constants and methods mostly for compatibility with previous Propel versions;
staabm
staabm Apr 19, 2013 Member

Map\ ? Maybe also drop it in this places?

marcj
marcj Apr 19, 2013 Owner

No, since it this just explains which classes are generated. Since there is no class \BookTableMap this would be wrong.

@staabm staabm commented on the diff Apr 19, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
@@ -1308,42 +1213,52 @@ protected function addLazyLoaderBody(&$script, Column $column)
\$c->addSelectColumn(".$this->getColumnConstant($column).");
try {
\$row = array(0 => null);
- \$stmt = ".$this->getQueryClassName()."::create(null, \$c)->setFormatter(ModelCriteria::FORMAT_STATEMENT)->find(\$con);
- \$stmt->bindColumn(1, \$row[0], PDO::PARAM_LOB, 0, PDO::SQLSRV_ENCODING_BINARY);
- \$stmt->fetch(PDO::FETCH_BOUND);
- \$stmt->closeCursor();";
+ \$dataFetcher = ".$this->getQueryClassName()."::create(null, \$c)->setFormatter(ModelCriteria::FORMAT_STATEMENT)->find(\$con);
+ if (\$dataFetcher instanceof PDODataFetcher) {
+ \$dataFetcher->bindColumn(1, \$row[0], PDO::PARAM_LOB, 0, PDO::SQLSRV_ENCODING_BINARY);
staabm
staabm Apr 19, 2013 Member

there is no bindColumn on the DataFetcher's

@staabm staabm commented on an outdated diff Apr 19, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
if ($column->getType() === PropelTypes::CLOB && $platform instanceof OraclePlatform) {
// PDO_OCI returns a stream for CLOB objects, while other PDO adapters return a string...
$script .= "
- \$this->$clo = stream_get_contents(\$row[0]);";
+ \$this->$clo = stream_get_contents(\$firstColumn);";
staabm
staabm Apr 19, 2013 Member

in case \$row is falsy in https://github.com/propelorm/Propel2/pull/359/files#L47R1235 this will lead to warnings. If I read the old code properly \$row should never be falsy -> you could strip the condition for it on https://github.com/propelorm/Propel2/pull/359/files#L47R1235

staabm
staabm Apr 19, 2013 Member

hmm also not related to your changes: I am wondering why $firstColumn is used with a null check in some of this conditions and in some not.. seems to be unnecessary

Member
staabm commented Apr 19, 2013

@marcj you have done a great job here, looks very promising!

The only thing I don't like is that, this PR is that huge. In case it is possible for you and doesn't just waste your time it may be a good idea to split it up in several PRs, which would ease reviewing it and also may reduce the risk to introduce some major problems...

Don't get me wrong, I see you already invested a lot here, therefore I will leave this up to you...

Owner
marcj commented Apr 19, 2013

@staabm, yes, I totally see your concerns - have the same here about that. But it's not that easy, because I can only split that PR in these commits. Peer Documentation, DataFetcher, populateObjects, OMBuilder and Peer general where still the last commit will still be very huge. Well but of course I can create for each a extra PR, that wouldn't be that much work and cleans it a bit. But containing those five commits in this PR here at the same time would be a mess to rebase later when fixing small stuff from further review. :)

Member
staabm commented Apr 19, 2013

Do you think we can get DataFetcher into a separate PR? since this is a completely new layer within propel2 this has a certain risk to break things and therefore would benefit a lot from a separate PR

(since this PR is so big I cannot estimate if it has to many dependencies on the other classes to separate it properly..)

Owner
marcj commented Apr 19, 2013

Ok, just reviewed it and saw that it would be really much work to split everything - also because the stuff need each other partially.

Member
staabm commented Apr 19, 2013

Ok, so we need all availabe pairs of eyeballs here ;)

Owner
marcj commented Apr 20, 2013

yupp :)

@jaugustin jaugustin commented on the diff Apr 21, 2013
UPDATE.md
@@ -40,13 +40,10 @@ The classes used by Propel internally to build the object model were renamed. Th
Replace... With...
OMBuilder.php AbstractOMBuilder.php
ObjectBuilder.php AbstractObjectBuilder.php
- PeerBuilder.php AbstractPeerBuilder.php
jaugustin
jaugustin Apr 21, 2013 Member

maybe instead of removing those lines we could add something that say bye, bye to Peer ;)

@jaugustin jaugustin commented on the diff Apr 21, 2013
...or/Sortable/SortableBehaviorObjectBuilderModifier.php
@@ -105,7 +102,7 @@ public function preDelete($builder)
$this->setBuilder($builder);
return "
-{$this->peerClassName}::shiftRank(-1, \$this->{$this->getColumnGetter()}() + 1, null, " . ($useScope ? "\$this->{$this->getColumnGetter('scope_column')}(), " : '') . "\$con);
+{$this->queryClassName}::sortableShiftRank(-1, \$this->{$this->getColumnGetter()}() + 1, null, " . ($useScope ? "\$this->{$this->getColumnGetter('scope_column')}(), " : '') . "\$con);
jaugustin
jaugustin Apr 21, 2013 Member

Why is this still static ?
Why did you change the method name change ? (I know we can introduce BC break, but I don't see the advantage of this one ;) )

marcj
marcj Apr 21, 2013 Owner

Because shiftRank is highly ambiguous.

jaugustin
jaugustin May 4, 2013 Member

@marcj could we postpone those changes ? cc @willdurand

@jaugustin jaugustin commented on the diff Apr 21, 2013
...or/Sortable/SortableBehaviorObjectBuilderModifier.php
@@ -341,7 +338,7 @@ public function insertAtRank(\$rank, ConnectionInterface \$con = null)
if (\$rank != \$maxRank + 1) {
// Keep the list modification query for the save() transaction
\$this->sortableQueries []= array(
- 'callable' => array('$peerClassName', 'shiftRank'),
+ 'callable' => array('{$queryClassName}', 'sortableShiftRank'),
jaugustin
jaugustin Apr 21, 2013 Member

same for all sortableShiftRank ;)

marcj
marcj Apr 27, 2013 Owner

see above.

@jaugustin jaugustin commented on the diff Apr 21, 2013
...ior/Sortable/SortableBehaviorQueryBuilderModifier.php
+ $useScope = $this->behavior->useScope();
+ $script .= "
+/**
+ * Get an item from the list based on its rank
+ *
+ * @param integer \$rank rank";
+ if ($useScope) {
+ $script .= "
+ * @param int \$scope Scope to determine which suite to consider";
+ }
+ $script .= "
+ * @param ConnectionInterface \$con optional connection
+ *
+ * @return {$this->objectClassName}
+ */
+static public function retrieveByRank(\$rank, " . ($useScope ? "\$scope = null, " : "") . "ConnectionInterface \$con = null)
jaugustin
jaugustin Apr 21, 2013 Member

this shouldn't existe anymore like this ;)

this should be

public function findByRank(...) // (not static ;) )

marcj
marcj Apr 27, 2013 Owner

see my newest comment at the bottom.

@jaugustin jaugustin commented on the diff Apr 21, 2013
...ior/Sortable/SortableBehaviorQueryBuilderModifier.php
+ $script .= "
+ * @param int \$scope Scope to determine which suite to consider";
+ }
+ $script .= "
+ * @param ConnectionInterface \$con optional connection
+ *
+ * @return {$this->objectClassName}
+ */
+static public function retrieveByRank(\$rank, " . ($useScope ? "\$scope = null, " : "") . "ConnectionInterface \$con = null)
+{
+ if (null === \$con) {
+ \$con = Propel::getServiceContainer()->getReadConnection({$this->tableMapClassName}::DATABASE_NAME);
+ }
+
+ \$c = new Criteria;
+ \$c->add({$this->tableMapClassName}::RANK_COL, \$rank);";
jaugustin
jaugustin Apr 21, 2013 Member

and we should use filter instead of this old Criteria

marcj
marcj Apr 27, 2013 Owner

see my newest comment at the bottom.

@jaugustin jaugustin commented on the diff Apr 21, 2013
...ior/Sortable/SortableBehaviorQueryBuilderModifier.php
+ }
+
+ protected function addDoSelectOrderByRank(&$script)
+ {
+ $queryClassName = $this->queryClassName;
+ $script .= "
+/**
+ * Return an array of sortable objects ordered by position
+ *
+ * @param Criteria \$criteria optional criteria object
+ * @param string \$order sorting order, to be chosen between Criteria::ASC (default) and Criteria::DESC
+ * @param ConnectionInterface \$con optional connection
+ *
+ * @return array list of sortable objects
+ */
+static public function doSelectOrderByRank(Criteria \$criteria = null, \$order = Criteria::ASC, ConnectionInterface \$con = null)
jaugustin
jaugustin Apr 21, 2013 Member

same here this is old peer way, maybe this could be removed or replaced by findOrderByRank(...)

marcj
marcj Apr 27, 2013 Owner

see my newest comment at the bottom.

@jaugustin jaugustin commented on the diff Apr 21, 2013
...ior/Sortable/SortableBehaviorQueryBuilderModifier.php
+";
+ }
+
+ protected function addRetrieveList(&$script)
+ {
+ $script .= "
+/**
+ * Return an array of sortable objects in the given scope ordered by position
+ *
+ * @param int \$scope the scope of the list
+ * @param string \$order sorting order, to be chosen between Criteria::ASC (default) and Criteria::DESC
+ * @param ConnectionInterface \$con optional connection
+ *
+ * @return array list of sortable objects
+ */
+static public function retrieveList(\$scope, \$order = Criteria::ASC, ConnectionInterface \$con = null)
jaugustin
jaugustin Apr 21, 2013 Member

same here

marcj
marcj Apr 27, 2013 Owner

see my newest comment at the bottom.

Member

@marcj I am doing a review, some of my comments could be done in an other PR ;)

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
$script .= "
try {
";
$n = 0;
foreach ($table->getColumns() as $col) {
if (!$col->isLazyLoad()) {
+
jaugustin
jaugustin Apr 21, 2013 Member

extra line not needed

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
@@ -1997,22 +1915,31 @@ protected function addHydrateBody(&$script)
{
$table = $this->getTable();
$platform = $this->getPlatform();
+
+ $tableMap= $this->getTableMapClassName();
jaugustin
jaugustin Apr 21, 2013 Member

CS space before =

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
$script .= "
try {
";
$n = 0;
foreach ($table->getColumns() as $col) {
if (!$col->isLazyLoad()) {
+
+ $indexName = "\$indexType == TableMap::TYPE_NUM ? $n + \$startcol : $tableMap::translateFieldName('{$col->getPhpName()}', TableMap::TYPE_PHPNAME, \$indexType)";
jaugustin
jaugustin Apr 21, 2013 Member

constant test first :
TableMap::TYPE_NUM == \$indexType

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
} elseif ($col->isLobType() && !$platform->hasStreamBlobImpl()) {
$script .= "
- if (\$row[\$startcol + $n] !== null) {
+ if (\$col !== null) {
jaugustin
jaugustin Apr 21, 2013 Member

same here reverse null and $col

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
}";
}
$script .= "
- \$this->$clo = (\$row[\$startcol + $n] !== null) ? PropelDateTime::newInstance(\$row[\$startcol + $n], null, '$dateTimeClass') : null;";
+ \$this->$clo = (\$col !== null) ? PropelDateTime::newInstance(\$col, null, '$dateTimeClass') : null;";
jaugustin
jaugustin Apr 21, 2013 Member

same here reverse null test

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
} elseif ($col->isPhpPrimitiveType()) {
$script .= "
- \$this->$clo = (\$row[\$startcol + $n] !== null) ? (".$col->getPhpType().") \$row[\$startcol + $n] : null;";
+ \$this->$clo = (\$col !== null) ? (".$col->getPhpType().") \$col : null;";
jaugustin
jaugustin Apr 21, 2013 Member

same here reverse null test

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
\$this->$cloUnserialized = null;";
} elseif ($col->isPhpObjectType()) {
$script .= "
- \$this->$clo = (\$row[\$startcol + $n] !== null) ? new ".$col->getPhpType()."(\$row[\$startcol + $n]) : null;";
+ \$this->$clo = (\$col !== null) ? new ".$col->getPhpType()."(\$col) : null;";
jaugustin
jaugustin Apr 21, 2013 Member

same here reverse null test

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
* @see doSave()
*/
- protected function doUpdate(ConnectionInterface \$con)
+ public function doUpdate(ConnectionInterface \$con)
jaugustin
jaugustin Apr 21, 2013 Member

Why is this public ? all do*() methods are protected, if this is needed maybe we should add an update() method

@jaugustin jaugustin and 2 others commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/QueryBuilder.php
@@ -497,11 +567,11 @@ protected function findPkSimple(\$key, \$con)
throw new PropelException(sprintf('Unable to execute SELECT statement [%s]', \$sql), 0, \$e);
}
\$obj = null;
- if (\$row = \$stmt->fetch(PDO::FETCH_NUM)) {";
+ if (\$row = \$stmt->fetch()) {";
@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Generator/Builder/Om/QueryBuilder.php
+ {
+ $table = $this->getTable();
+ $script .= "
+ /**
+ * Deletes all rows from the ".$table->getName()." table.
+ *
+ * @param ConnectionInterface \$con the connection to use
+ * @return int The number of affected rows (if supported by underlying database driver).
+ */
+ public function doDeleteAll(ConnectionInterface \$con = null)
+ {
+ if (null === \$con) {
+ \$con = Propel::getServiceContainer()->getWriteConnection(" . $this->getTableMapClass() . "::DATABASE_NAME);
+ }
+ \$affectedRows = 0; // initialize var to track total num of affected rows
+ try {
jaugustin
jaugustin Apr 21, 2013 Member

the try catch rollback is already done in the deleteAll() method, this is redundant ?

marcj
marcj Apr 27, 2013 Owner

True there is also a try, but the other stuff in this block should be caught, too.

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
@@ -375,11 +408,35 @@ protected function addClassClose(&$script)
{
$script .= "
} // " . $this->getUnqualifiedClassName() . "
+// This is the static code needed to register the TableMap for this table with the main Propel class.
+//
+".$this->getUnqualifiedClassName()."::buildTableMap();
jaugustin
jaugustin Apr 21, 2013 Member

this is very old school trick ;) I will create a RFC for that

staabm
staabm Apr 27, 2013 Member

This also is against psr-0

marcj
marcj Apr 27, 2013 Owner

It's just copied from the peer class. Let's discuss this in another issue.

staabm
staabm Apr 27, 2013 Member

Sure, @jaugustin will create the RFC

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
@@ -716,7 +774,7 @@ public static function getPrimaryKeyHashFromRow(\$row, \$startcol = 0)
foreach ($this->getTable()->getColumns() as $col) {
if (!$col->isLazyLoad()) {
if ($col->isPrimaryKey()) {
- $part = $n ? "\$row[\$startcol + $n]" : "\$row[\$startcol]";
+ $part = "\$row[\$indexType == TableMap::TYPE_NUM ? $n + \$offset : static::translateFieldName('{$col->getPhpName()}', TableMap::TYPE_PHPNAME, \$indexType)]";
jaugustin
jaugustin Apr 21, 2013 Member

reverse constant test

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
} else {
+
+ $pk = "''";
+ foreach ($table->getColumns() as $col) {
jaugustin
jaugustin Apr 21, 2013 Member

I don't like this duplication of code ;)

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
...nerator/Builder/Om/templates/tableMapInstancePool.php
@@ -33,12 +33,17 @@ public static function addInstanceToPool($obj, $key = null)
public static function removeInstanceFromPool($value)
{
if (Propel::isInstancePoolingEnabled() && null !== $value) {
+
jaugustin
jaugustin Apr 21, 2013 Member

extra line ?

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Command/SqlInsertCommand.php
@@ -42,7 +43,9 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
+
jaugustin
jaugustin Apr 21, 2013 Member

remove extra line

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Config/GeneratorConfig.php
@@ -151,7 +151,7 @@ public function getClassName($propname)
*/
public function getBuilderClassName($type)
{
- $propname = 'builder' . ucfirst(strtolower($type)) . 'Class';
+ $propname = 'builder' . ucfirst(strtolower($type)). 'Class';
jaugustin
jaugustin Apr 21, 2013 Member

why this change ?

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Manager/MigrationManager.php
@@ -121,7 +120,13 @@ public function getOldestDatabaseVersion()
$oldestMigrationTimestamp = null;
$migrationTimestamps = array();
foreach ($connections as $name => $params) {
+
jaugustin
jaugustin Apr 21, 2013 Member

remove extra line

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
// First load the tables (important that this happen before filling out details of tables)
$tables = array();
- while ($row = $stmt->fetch(\PDO::FETCH_NUM)) {
+ foreach ($dataFetcher as $row){
jaugustin
jaugustin Apr 21, 2013 Member

CS 4 spaces

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
- while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) {
+ foreach ($dataFetcher as $row) {
jaugustin
jaugustin Apr 21, 2013 Member

same here CS 4 spaces

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
@@ -169,7 +169,7 @@ protected function addForeignKeys(Table $table)
WHERE (ccu1.table_name = '".$table->getName()."')");
$foreignKeys = array(); // local store to avoid duplicates
- while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) {
+ foreach ($dataFetcher as $row) {
jaugustin
jaugustin Apr 21, 2013 Member

same here CS 4 spaces

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
$indexes = array();
- while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) {
+ foreach ($dataFetcher as $row) {
jaugustin
jaugustin Apr 21, 2013 Member

same here CS 4 spaces

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
@@ -229,7 +229,7 @@ protected function addPrimaryKey(Table $table)
// Loop through the returned results, grouping the same key_name together
// adding each column for that key.
- while ($row = $stmt->fetch(\PDO::FETCH_NUM)) {
+ foreach ($dataFetcher as $row) {
jaugustin
jaugustin Apr 21, 2013 Member

same here CS 4 spaces

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Generator/Reverse/MysqlSchemaParser.php
// First load the tables (important that this happen before filling out details of tables)
$tables = array();
- while ($row = $stmt->fetch(\PDO::FETCH_NUM)) {
+ foreach ($dataFetcher as $row) {
jaugustin
jaugustin Apr 21, 2013 Member

same here CS 4 spaces

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Generator/Reverse/SqliteSchemaParser.php
// First load the tables (important that this happen before filling out details of tables)
$tables = array();
- while ($row = $stmt->fetch(\PDO::FETCH_NUM)) {
+ foreach ($dataFetcher as $row) {
jaugustin
jaugustin Apr 21, 2013 Member

same here CS 4 spaces

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/BaseModelCriteria.php
+use Propel\Runtime\Formatter\AbstractFormatter;
+use Propel\Runtime\Map\TableMap;
+use Propel\Runtime\ActiveQuery\Criteria;
+
+class BaseModelCriteria extends Criteria implements \IteratorAggregate
+{
+ protected $modelName;
+
+ protected $modelTableMapName;
+
+ protected $modelAlias;
+
+ protected $tableMap;
+
+ protected $formatter;
+ protected $with = array();
jaugustin
jaugustin Apr 21, 2013 Member

add extra line before, maybe also add phpdoc on all properties

marcj
marcj Apr 27, 2013 Owner

From old code, but fixed that missing line.

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/BaseModelCriteria.php
+ protected $defaultFormatterClass = ModelCriteria::FORMAT_OBJECT;
+
+ /**
+ * Creates a new instance with the default capacity which corresponds to
+ * the specified database.
+ *
+ * @param string $dbName The dabase name
+ * @param string $modelName The phpName of a model, e.g. 'Book'
+ * @param string $modelAlias The alias for the model in this query, e.g. 'b'
+ */
+ public function __construct($dbName = null, $modelName = null, $modelAlias = null)
+ {
+ $this->setDbName($dbName);
+ $this->originalDbName = $dbName;
+ $this->setModelName($modelName);
+ $this->modelAlias = $modelAlias;
jaugustin
jaugustin Apr 21, 2013 Member

extra space before = ?

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/BaseModelCriteria.php
+ return $this->modelName;
+ }
+
+ public function setModelName($modelName)
+ {
+ if (0 === strpos($modelName, '\\')) {
+ $this->modelName = substr($modelName, 1);
+ } else {
+ $this->modelName = $modelName;
+ }
+ if ($this->modelName && !$this->modelTableMapName) {
+ $this->modelTableMapName = constant($this->modelName . '::TABLE_MAP');
+ }
+ if (!$this->tableMap && $this->modelName) {
+ $this->tableMap = Propel::getServiceContainer()->getDatabaseMap($this->getDbName())->getTableByPhpName($this->modelName);
+ }
jaugustin
jaugustin Apr 21, 2013 Member

all setter are fluid ? maybe we should return $this

@jaugustin jaugustin and 1 other commented on an outdated diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
@@ -790,6 +790,11 @@ public function add($p1, $value = null, $comparison = null)
return $this;
}
+ public function __toString()
+ {
+ return "" . implode(', ', $this->map) . "\n";
jaugustin
jaugustin Apr 21, 2013 Member

is "" . necessary ?

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ $joinClause = array();
+ $joinTables = array();
+ $whereClause = array();
+ $orderByClause = array();
+
+ $orderBy = $this->getOrderByColumns();
+ $groupBy = $this->getGroupByColumns();
+
+ // get the first part of the SQL statement, the SELECT part
+ $selectSql = $db->createSelectSqlPart($this, $fromClause);
+
+ // Handle joins
+ // joins with a null join type will be added to the FROM clause and the condition added to the WHERE clause.
+ // joins of a specified type: the LEFT side will be added to the fromClause and the RIGHT to the joinClause
+ foreach ($this->getJoins() as $join) {
+
jaugustin
jaugustin Apr 21, 2013 Member

remove extra line

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+
+ $join->setAdapter($db);
+
+ // add 'em to the queues..
+ if (!$fromClause) {
+ $fromClause[] = $join->getLeftTableWithAlias();
+ }
+ $joinTables[] = $join->getRightTableWithAlias();
+ $joinClause[] = $join->getClause($params);
+ }
+
+ // add the criteria to WHERE clause
+ // this will also add the table names to the FROM clause if they are not already
+ // included via a LEFT JOIN
+ foreach ($this->keys() as $key) {
+
jaugustin
jaugustin Apr 21, 2013 Member

remove extra line

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ $joinTables[] = $join->getRightTableWithAlias();
+ $joinClause[] = $join->getClause($params);
+ }
+
+ // add the criteria to WHERE clause
+ // this will also add the table names to the FROM clause if they are not already
+ // included via a LEFT JOIN
+ foreach ($this->keys() as $key) {
+
+ $criterion = $this->getCriterion($key);
+ $table = null;
+ foreach ($criterion->getAttachedCriterion() as $attachedCriterion) {
+ $tableName = $attachedCriterion->getTable();
+
+ $table = $this->getTableForAlias($tableName);
+ if ($table !== null) {
jaugustin
jaugustin Apr 21, 2013 Member

reverse null test

marcj
marcj Apr 27, 2013 Owner

From old code.

@jaugustin jaugustin commented on an outdated diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+
+ // add 'em to the queues..
+ if (!$fromClause) {
+ $fromClause[] = $join->getLeftTableWithAlias();
+ }
+ $joinTables[] = $join->getRightTableWithAlias();
+ $joinClause[] = $join->getClause($params);
+ }
+
+ // add the criteria to WHERE clause
+ // this will also add the table names to the FROM clause if they are not already
+ // included via a LEFT JOIN
+ foreach ($this->keys() as $key) {
+
+ $criterion = $this->getCriterion($key);
+ $table = null;
jaugustin
jaugustin Apr 21, 2013 Member

useless ;) I think

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ }
+ }
+
+ // Add the GROUP BY columns
+ $groupByClause = $groupBy;
+
+ $having = $this->getHaving();
+ $havingString = null;
+ if (null !== $having) {
+ $sb = '';
+ $having->appendPsTo($sb, $params);
+ $havingString = $sb;
+ }
+
+ if (!empty($orderBy)) {
+
jaugustin
jaugustin Apr 21, 2013 Member

remove extra line

marcj
marcj Apr 27, 2013 Owner

From old code.

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+
+ // Add the GROUP BY columns
+ $groupByClause = $groupBy;
+
+ $having = $this->getHaving();
+ $havingString = null;
+ if (null !== $having) {
+ $sb = '';
+ $having->appendPsTo($sb, $params);
+ $havingString = $sb;
+ }
+
+ if (!empty($orderBy)) {
+
+ foreach ($orderBy as $orderByColumn) {
+
jaugustin
jaugustin Apr 21, 2013 Member

same here remove extra line

marcj
marcj Apr 27, 2013 Owner

From old code.

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ $havingString = $sb;
+ }
+
+ if (!empty($orderBy)) {
+
+ foreach ($orderBy as $orderByColumn) {
+
+ // Add function expression as-is.
+
+ if (strpos($orderByColumn, '(') !== false) {
+ $orderByClause[] = $orderByColumn;
+ continue;
+ }
+
+ // Split orderByColumn (i.e. "table.column DESC")
+
jaugustin
jaugustin Apr 21, 2013 Member

remove extra line

marcj
marcj Apr 27, 2013 Owner

From old code.

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+
+ $having = $this->getHaving();
+ $havingString = null;
+ if (null !== $having) {
+ $sb = '';
+ $having->appendPsTo($sb, $params);
+ $havingString = $sb;
+ }
+
+ if (!empty($orderBy)) {
+
+ foreach ($orderBy as $orderByColumn) {
+
+ // Add function expression as-is.
+
+ if (strpos($orderByColumn, '(') !== false) {
jaugustin
jaugustin Apr 21, 2013 Member

reverse false test

marcj
marcj Apr 27, 2013 Owner

From old code.

@jaugustin jaugustin commented on the diff Apr 21, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ if (!empty($orderBy)) {
+
+ foreach ($orderBy as $orderByColumn) {
+
+ // Add function expression as-is.
+
+ if (strpos($orderByColumn, '(') !== false) {
+ $orderByClause[] = $orderByColumn;
+ continue;
+ }
+
+ // Split orderByColumn (i.e. "table.column DESC")
+
+ $dotPos = strrpos($orderByColumn, '.');
+
+ if ($dotPos !== false) {
jaugustin
jaugustin Apr 21, 2013 Member

reverse false test

marcj
marcj Apr 27, 2013 Owner

From old code.

Owner
marcj commented Apr 21, 2013

@jaugustin, The most code comes from copy&paste, so I don't think it makes much sense to fix/mention now every CS issue of the old code - and it's actually also not part of this PR.

Member

@marcj ok I stop commenting CS ;)

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Runtime/DataFetcher/PDODataFetcher.php
+class PDODataFetcher extends AbstractDataFetcher
+{
+ /**
+ * @var array
+ */
+ private $current;
+
+ /**
+ * @var int
+ */
+ private $index = 0;
+
+ /**
+ * {@inheritDoc}
+ */
+ public function fetch($style = \PDO::FETCH_NUM)
staabm
staabm Apr 27, 2013 Member

signature is not compatible with interface (no param in the interface)

marcj
marcj Apr 27, 2013 Owner

That's not necessary.

staabm
staabm Apr 27, 2013 Member

but won't it emit a E_STRICT warning?

@staabm staabm and 1 other commented on an outdated diff Apr 27, 2013
src/Propel/Generator/Builder/Om/ObjectBuilder.php
@@ -1968,9 +1882,13 @@ protected function addHydrateComment(&$script)
* for results of JOIN queries where the resultset row includes columns from two or
* more tables.
*
- * @param array \$row The row returned by Statement->fetch(PDO::FETCH_NUM)
- * @param int \$startcol 0-based offset column which indicates which restultset column to start with.
- * @param boolean \$rehydrate Whether this object is being re-hydrated from the database.
+ * @param array \$row The row returned by DataFetcher->fetch().
staabm
staabm Apr 27, 2013 Member

per interface ->fetch() returns mixed|null

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Builder/Om/QueryBuilder.php
+ \$c->doOnDeleteSetNull(\$con);
+ ";
+ }
+
+ $script .= "
+
+ {$this->getTableMapClassName()}::removeInstanceFromPool(\$criteria);
+ ";
+
+ $script .= "
+ \$affectedRows += ModelCriteria::delete(\$con);
+ {$this->getTableMapClassName()}::clearRelatedInstancePool();
+ \$con->commit();
+
+ return \$affectedRows;
+ } catch (PropelException \$e) {
staabm
staabm Apr 27, 2013 Member

It should rollback no matter which Exception occured?

marcj
marcj Apr 27, 2013 Owner

From old code.

staabm
staabm Apr 27, 2013 Member

I see.. checked Propel1 and there is also this error... will create a separate issue

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Builder/Om/QueryBuilder.php
+ *
+ * This method is not very speedy because it must perform a query first to get
+ * the implicated records and then perform the deletes by calling those Query classes.
+ *
+ * This method should be used within a transaction if possible.
+ *
+ * @param ConnectionInterface \$con
+ * @return int The number of affected rows (if supported by underlying database driver).
+ */
+ protected function doOnDeleteCascade(ConnectionInterface \$con)
+ {
+ // initialize var to track total num of affected rows
+ \$affectedRows = 0;
+
+ // first find the objects that are implicated by the \$this
+ \$objects = {$this->getQueryClassName()}::create(null, \$this)->find(\$con);
staabm
staabm Apr 27, 2013 Member

this is a $dataFetcher?

marcj
marcj Apr 27, 2013 Owner

Nope, it's a collection.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Builder/Om/QueryBuilder.php
+ $script .="\$affectedRows += \$this->doOnDeleteCascade(\$con);
+ ";
+ }
+ if ($this->isDeleteSetNullEmulationNeeded()) {
+ $script .= "\$this->doOnDeleteSetNull(\$con);
+ ";
+ }
+ $script .= "\$affectedRows += parent::doDeleteAll(\$con);
+ // Because this db requires some delete cascade/set null emulation, we have to
+ // clear the cached instance *after* the emulation has happened (since
+ // instances get re-added by the select statement contained therein).
+ {$this->getTableMapClassName()}::clearInstancePool();
+ {$this->getTableMapClassName()}::clearRelatedInstancePool();
+
+ \$con->commit();
+ } catch (PropelException \$e) {
staabm
staabm Apr 27, 2013 Member

rollback on any exception type..?

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
...opel/Generator/Builder/Om/QueryInheritanceBuilder.php
{
// condition on class key is already added in preDelete()
- return parent::doDelete(\$con);
+ return parent::delete(\$con);
staabm
staabm Apr 27, 2013 Member

intentionally changed from doDelete()? to delete()?

marcj
marcj Apr 27, 2013 Owner

Yes, because delete does more than doDelete.

@staabm staabm and 1 other commented on an outdated diff Apr 27, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
+ \$cls = preg_replace('#\.#', '\\\\', \$cls);
+ " . $this->buildObjectInstanceCreationCode('$obj', '$cls') . "
+ \$obj->hydrate(\$row);
+ \$results[] = \$obj;
+ {$this->getTableMapClassName()}::addInstanceToPool(\$obj, \$key);";
+ } else {
+ $script .= "
+ " . $this->buildObjectInstanceCreationCode('$obj', '$cls') . "
+ \$obj->hydrate(\$row);
+ \$results[] = \$obj;
+ {$this->getTableMapClassName()}::addInstanceToPool(\$obj, \$key);";
+ }
+ $script .= "
+ } // if key exists
+ }
+ \$dataFetcher->close();
staabm
staabm Apr 27, 2013 Member

shouldn't the dataFetcher be closed by the caller of this method, because the caller created this object...?

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
+ }
+";
+ }
+
+ /**
+ * Adds the doDelete() method.
+ * @param string &$script The script will be modified in this method.
+ */
+ protected function addDoDelete(&$script)
+ {
+ $table = $this->getTable();
+ $script .= "
+ /**
+ * Performs a DELETE on the database, given a ".$this->getObjectClassName()." or Criteria object OR a primary key value.
+ *
+ * @param mixed \$values Criteria or ".$this->getObjectClassName()." object or primary key or array of primary keys
staabm
staabm Apr 27, 2013 Member

use Criteria|".$this->getObjectClassName()|mixed instead of mixed to help IDEs

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
+ && $col->isAutoIncrement()
+ && 'none' !== $table->getIdMethod()
+ && !$table->isAllowPkInsert()
+ ) {
+ $script .= "
+ if (\$criteria->containsKey(".$this->getColumnConstant($col).") && \$criteria->keyContainsValue(" . $this->getColumnConstant($col) . ") ) {
+ throw new PropelException('Cannot insert a value for auto-increment primary key ('.".$this->getColumnConstant($col).".')');
+ }
+";
+ if (!$this->getPlatform()->supportsInsertNullPk()) {
+ $script .= "
+ // remove pkey col since this table uses auto-increment and passing a null value for it is not valid
+ \$criteria->remove(".$this->getColumnConstant($col).");
+";
+ }
+ } elseif ($col->isPrimaryKey()
staabm
staabm Apr 27, 2013 Member

better refactor the 3 common conditions into a separate if to ease readability

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Builder/Om/TableMapBuilder.php
+";
+ }
+ }
+
+ $script .= "
+
+ // Set the correct dbName
+ \$query = " . $this->getQueryClassName() . "::create()->mergeWith(\$criteria);
+
+ try {
+ // use transaction because \$criteria could contain info
+ // for more than one table (I guess, conceivably)
+ \$con->beginTransaction();
+ \$pk = \$query->doInsert(\$con);
+ \$con->commit();
+ } catch (PropelException \$e) {
staabm
staabm Apr 27, 2013 Member

rollback on any exception type

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Platform/DefaultPlatform.php
@@ -85,6 +90,17 @@ public function getConnection()
return $this->con;
}
+ public function getBuilderClass($type)
+ {
+ $class = get_called_class();
+ $class = substr($class, strrpos($class, '\\') + 1, -(strlen('Platform')));
+ $class = 'Propel\Generator\Builder\Om\Platform\\' . $class . ucfirst($type) . 'Builder';
+
+ if (class_exists($class)) {
+ return $class;
+ }
staabm
staabm Apr 27, 2013 Member

throw exception in case no proper builder can be found?

marcj
marcj Apr 27, 2013 Owner

No, because it's optional. Added more doc.

@staabm staabm and 1 other commented on an outdated diff Apr 27, 2013
src/Propel/Generator/Platform/DefaultPlatform.php
@@ -85,6 +90,17 @@ public function getConnection()
return $this->con;
}
+ public function getBuilderClass($type)
+ {
+ $class = get_called_class();
+ $class = substr($class, strrpos($class, '\\') + 1, -(strlen('Platform')));
staabm
staabm Apr 27, 2013 Member

looks like a lot of guess-work is in the game

marcj
marcj Apr 27, 2013 Owner

That's deliberate.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Platform/PgsqlPlatform.php
@@ -502,9 +502,8 @@ public function getIdentifierPhp($columnValueMutator, $connectionVariableName =
throw new EngineException('PostgreSQL needs a sequence name to fetch primary keys');
}
$snippet = "
-\$stmt = %s->query(\"SELECT nextval('%s')\");
-\$row = \$stmt->fetch(\\PDO::FETCH_NUM);
-%s = \$row[0];";
+\$dataFetcher = %s->query(\"SELECT nextval('%s')\");
@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Platform/OraclePlatform.php
@@ -380,9 +380,8 @@ public function getIdentifierPhp($columnValueMutator, $connectionVariableName =
throw new EngineException('Oracle needs a sequence name to fetch primary keys');
}
$snippet = "
-\$stmt = %s->query('SELECT %s.nextval FROM dual');
-\$row = \$stmt->fetch(PDO::FETCH_NUM);
-%s = \$row[0];";
+\$dataFetcher = %s->query('SELECT %s.nextval FROM dual');
staabm
staabm Apr 27, 2013 Member

no dataFetcher .. PDOStatement here

staabm
staabm Apr 27, 2013 Member

hmm the ConnectionInterface also does not have a query method

marcj
marcj Apr 27, 2013 Owner

Its from SQLConnectionInterface implemented. So, that's correct. https://github.com/propelorm/Propel2/pull/359/files#L100R96

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
@@ -78,11 +78,11 @@ protected function getTypeMapping()
public function parse(Database $database, Task $task = null)
{
- $stmt = $this->dbh->query("SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE = 'BASE TABLE' AND TABLE_NAME <> 'dtproperties'");
+ $dataFetcher = $this->dbh->query("SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE = 'BASE TABLE' AND TABLE_NAME <> 'dtproperties'");
staabm
staabm Apr 27, 2013 Member

same as above

staabm
staabm Apr 27, 2013 Member

than you need to Adjust the type of $this->dbh in the AbstractSchemaParser from ConnectionInterface to SqlConnectionInterface

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Generator/Reverse/MssqlSchemaParser.php
@@ -115,9 +115,9 @@ public function parse(Database $database, Task $task = null)
*/
protected function addColumns(Table $table)
{
- $stmt = $this->dbh->query("sp_columns '" . $table->getName() . "'");
+ $dataFetcher = $this->dbh->query("sp_columns '" . $table->getName() . "'");
@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Runtime/ActiveQuery/BaseModelCriteria.php
+ * @return array
+ */
+ public function getWith()
+ {
+ return $this->with;
+ }
+
+ /**
+ * Sets the array of ModelWith specifying which objects must be hydrated
+ * together with the main object.
+ *
+ * @param array
+ *
+ * @return ModelCriteria The current object, for fluid interface
+ */
+ public function setWith($with)
marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ // which is 0. Fixes a bug in which an UPDATE statement
+ // would fail in this instance.
+
+ if (empty($updateTablesColumns)) {
+ return 0;
+ }
+
+ $affectedRows = 0; // initialize this in case the next loop has no iterations.
+
+ foreach ($tablesColumns as $tableName => $columns) {
+
+ $whereClause = array();
+ $params = array();
+ $stmt = null;
+ try {
+ $sql = 'UPDATE ';
staabm
staabm Apr 27, 2013 Member

do we need a transaction inside the foreach loop to make it atomic?

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Runtime/ActiveQuery/Criteria.php
+ // Set up a list of required tables (one DELETE statement will
+ // be executed per table)
+ $tables = $this->getTablesColumns();
+ if (empty($tables)) {
+ throw new PropelException("Cannot delete from an empty Criteria");
+ }
+
+ $affectedRows = 0; // initialize this in case the next loop has no iterations.
+
+ foreach ($tables as $tableName => $columns) {
+
+ $whereClause = array();
+ $params = array();
+ $stmt = null;
+ try {
+ $sql = $db->getDeleteFromClause($this, $tableName);
staabm
staabm Apr 27, 2013 Member

transaction?

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm commented on the diff Apr 27, 2013
...l/Runtime/ActiveQuery/Criterion/AbstractCriterion.php
@@ -275,6 +275,15 @@ public function appendPsTo(&$sb, array &$params)
}
}
+ public function __toString()
+ {
+ $sb = '';
+ $params = [];
staabm
staabm Apr 27, 2013 Member

love the new PHP 5.4's array notation.. @willdurand is it ok per CS?

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Runtime/ActiveQuery/InstancePoolTrait.php
}
self::$instances[$key] = $object;
}
}
+ public static function getInstanceKey($value)
+ {
+ if (!($value instanceof Criteria) && is_object($value)) {
staabm
staabm Apr 27, 2013 Member

what about criteria objects here? when a Criteria is passed into this function it will return null because no explicit return is reached. I think we should throw a exception in case we cannot calc a key for the given $value because otherwise it will always be null and therefore override each other

marcj
marcj Apr 27, 2013 Owner

No, that's ok, because removeInstanceFromPool works with that null value. The only thing I can see here is with addInstancePool that it overwrites null values, but I guess that's also ok, because in the old code it throws a fatal error, because of this https://github.com/propelorm/Propel2/blob/master/src/Propel/Runtime/ActiveQuery/InstancePoolTrait.php#L24
So it looks like it already has been made sure that the $object for addInstanceToPool is the correct type.

Anyways, we should add there more docblocks and look through that code - but in a other #issue, please.

@staabm staabm commented on the diff Apr 27, 2013
src/Propel/Runtime/ActiveQuery/ModelCriteria.php
@@ -1640,36 +1327,23 @@ public function delete($con = null)
$con->commit();
} catch (PropelException $e) {
staabm
staabm Apr 27, 2013 Member

any exception type

marcj
marcj Apr 27, 2013 Owner

From old code.

marcj
marcj Apr 27, 2013 Owner

From old code.

@staabm staabm referenced this pull request Apr 27, 2013
Closed

Transaction leak #365

Owner
marcj commented Apr 27, 2013

Guys, of course I've had to copy a lot of old code around, but please don't review now that old code, because I can't make now all old moved code pretty, also because that's not the part of this PR. This PR is (also) about removing the peer class, not about making all moved code prettier or adjust behaviours methods to a new CS (which of course comes with the peer removing, but shouldn't be a part of this PR).

It makes me much more work to look through your invalid/unnecessary reviews of this code pieces. Please make yourself sure that you are checking new code before you throw out comments. That saves me time and doesn't blow up the PR with useless comments. :)

Member
staabm commented Apr 27, 2013

sorry for those comments on the old code.. the PR is that huge that it is not easy to distinguish between whats new and whats old.

I also don't want you to adjust old code issues, those should be handled in a separate issue (like we already did for some of them)

Owner

yep, sorry for the comments on the old code.

Owner
marcj commented May 2, 2013

Ok folks, how's the status of this PR?

Owner

It looks good to me. A few things though:

  • this seems not easily mergeable, rebase?
  • do you think it's worth squashing your commits?
  • @jaugustin HAS to give his "go"
Owner
marcj commented May 3, 2013

Yes, I'll squash/rebase it if anything is good.

Member

@willdurand I reviewed 50% of this huge PR, I will do the last half this weedend ;)

@jaugustin jaugustin and 1 other commented on an outdated diff May 4, 2013
src/Propel/Runtime/ActiveQuery/ModelCriteria.php
- } else {
- // Replace SELECT columns with COUNT(*)
- $this->clearSelectColumns()->addSelectColumn('COUNT(*)');
- $sql = BasePeer::createSelectSql($this, $params);
- }
- try {
- $stmt = $con->prepare($sql);
- $db->bindValues($stmt, $params, $dbMap);
- $stmt->execute();
- } catch (Exception $e) {
- Propel::log($e->getMessage(), Propel::LOG_ERR);
- throw new PropelException(sprintf('Unable to execute COUNT statement [%s]', $sql), 0, $e);
- }
-
- return $stmt;
+ return parent::doCount($con = null);
jaugustin
jaugustin May 4, 2013 Member

why you set $con = null ?
this should be return parent::doCount($con)

marcj
marcj May 10, 2013 Owner

typo, fixed.

@jaugustin jaugustin commented on the diff May 4, 2013
src/Propel/Runtime/ActiveQuery/ModelCriteria.php
return $count;
}
- protected function doCount($con)
+ public function doCount($con = null)
jaugustin
jaugustin May 4, 2013 Member

ConnectionInterface ?

@jaugustin jaugustin and 1 other commented on an outdated diff May 4, 2013
src/Propel/Runtime/DataFetcher/AbstractDataFetcher.php
+ * {@inheritDoc}
+ */
+ public function getDataObject()
+ {
+ return $this->dataObject;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function fetchColumn($index = null)
+ {
+ $next = $this->fetch();
+
+ if ($next) {
+ return null === $index ? current($next) : $next[$index];
jaugustin
jaugustin May 4, 2013 Member

no test if $index exists ?

@jaugustin jaugustin and 1 other commented on an outdated diff May 4, 2013
src/Propel/Runtime/DataFetcher/ArrayDataFetcher.php
+ *
+ * @package Propel\Runtime\Formatter
+ */
+class ArrayDataFetcher extends AbstractDataFetcher
+{
+ /**
+ * @var string
+ */
+ protected $indexType = TableMap::TYPE_PHPNAME;
+
+ /**
+ * {@inheritDoc}
+ */
+ public function next()
+ {
+ null === $this->dataObject ? : next($this->dataObject);
jaugustin
jaugustin May 4, 2013 Member

add a if statement

@jaugustin jaugustin commented on an outdated diff May 4, 2013
src/Propel/Runtime/DataFetcher/ArrayDataFetcher.php
+ protected $indexType = TableMap::TYPE_PHPNAME;
+
+ /**
+ * {@inheritDoc}
+ */
+ public function next()
+ {
+ null === $this->dataObject ? : next($this->dataObject);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function current()
+ {
+ return null === $this->dataObject ? : current($this->dataObject);
jaugustin
jaugustin May 4, 2013 Member

if dataObject is null we return true is there a reason for that ? false would have been better

@jaugustin jaugustin and 1 other commented on an