Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added multiple scope support in Sortable behavior. #616

Merged
merged 1 commit into from Apr 28, 2013

Conversation

Projects
None yet
5 participants
Owner

marcj commented Feb 23, 2013

Todos:

  • Add more than one test.
  • Add updated documentation.

and probably other stuff, too.

This PR should be stay open until the propel documentation has been updated.

Owner

marcj commented Feb 23, 2013

@staabm, @jaugustin poll:

  1. Define multiple scopes through comma separated list (as it is now)
  2. Define multiple scopes through multiple <parameter> elements

which way is better - what do you think?

@staabm, could you help out with the documentation part? Am a bit busy with the sqlite migration PR already.

Member

staabm commented Feb 23, 2013

How would the notation look like with several parameter tags?

    <parameter name="scope_column1" value="user_id" />
    <parameter name="scope_column2" value="group_id" />

?

I cant think of a good solution using several parameter tags...

@staabm staabm closed this Feb 23, 2013

@staabm staabm reopened this Feb 23, 2013

Member

staabm commented Feb 23, 2013

Ups sorry. Wrong button clicked..

Member

staabm commented Feb 23, 2013

Could do the documentation after we found out how things should work..

Just to get it right: multiple scopes mean that one sortable list is managed for each combination of the scope columns provided? Are Nulls and therefore their combinations allowed?

Member

jaugustin commented Feb 23, 2013

@marcj I think the coma choice is the good one ;)

Owner

marcj commented Feb 23, 2013

@staabm, yes, if you allow NULLs in one column of the composite scope, then this creates a additionally combination.

Ok, then I let it as it is.

Member

staabm commented Feb 23, 2013

Will try to find some time to get the docs prepared on sunday...

Owner

marcj commented Feb 23, 2013

👍

@staabm staabm commented on an outdated diff Feb 24, 2013

...vior/sortable/SortableBehaviorPeerBuilderModifier.php
@@ -67,15 +77,6 @@ public function staticAttributes($builder)
const RANK_COL = '" . $tableName . '.' . $this->getColumnConstant('rank_column') . "';
";
- if ($this->behavior->useScope()) {
- $script .= "
-/**
- * Scope column for the set
- */
-const SCOPE_COL = '" . $tableName . '.' . $this->getColumnConstant('scope_column') . "';
@staabm

staabm Feb 24, 2013

Member

won't this break BC for sortable-scoped with 1 column because this const is no longer available?

Member

staabm commented Feb 24, 2013

@marcj I checked out the source of this PR and triggered a build locally... your work seems not be finished?

Nevertheless I started documenting how I would suppose the API will look like. After I got your GO I will finish the docs.

Another open point which should be added to the ToDo list of this PR: "create issue to port this enhancement into Propel2"

@staabm staabm referenced this pull request in propelorm/propelorm.github.com Feb 24, 2013

Closed

docs of multi scope sortable behavior #203

Owner

marcj commented Feb 24, 2013

Yupp, not done yet. Maybe I get it done when I'm home this evening.

Member

staabm commented Feb 24, 2013

No hurry, it's done when it's done.

Owner

marcj commented Feb 24, 2013

;)

@jaugustin jaugustin and 1 other commented on an outdated diff Feb 25, 2013

...or/sortable/SortableBehaviorObjectBuilderModifier.php
@@ -18,7 +18,17 @@
*/
class SortableBehaviorObjectBuilderModifier
{
- protected $behavior, $table, $builder, $objectClassname, $peerClassname;
+ /**
+ * @var SortableBehavior
+ */
+ protected $behavior;
+
+ /**
+ * @var Table
+ */
+ protected $table;
+
+ protected $builder, $objectClassname, $peerClassname;
@jaugustin

jaugustin Feb 25, 2013

Member

I think you could add doc block to all properties ;)

@marcj

marcj Feb 26, 2013

Owner

yo, how should I resist there if you ask me so nicely. ;)

@marcj marcj referenced this pull request in propelorm/Propel2 Feb 26, 2013

Closed

Port PR #616 from Propel 1.6 #347

Owner

marcj commented Feb 26, 2013

@staabm, the method signatures are done now. If you could please do a review, then it should be ready to merge. Port PR issue is also created propelorm/Propel2#347.
The documention part looks awesome so far. I guess this will be a fast merge then.

@staabm staabm commented on the diff Feb 26, 2013

generator/lib/behavior/sortable/SortableBehavior.php
}
}
}
}
+ /**
+ * Generates the method argument signature, the appropriate phpDoc for @params,
+ * the scope builder php code and the scope variable builder php code/
+ *
+ * @return array ($methodSignature, $paramsDoc, $scopeBuilder, $buildScopeVars)
@staabm

staabm Feb 26, 2013

Member

Dont think this is valid phpdoc.. array(string, int,.. would work

@staabm staabm commented on an outdated diff Feb 26, 2013

...ortableBehaviorObjectBuilderModifierWithScopeTest.php
+
+ $result = array();
+ foreach ($items as $value) {
+ $item = new SortableMultiScopes();
+ $item->setCategoryId($value[0]);
+ $item->setSubCategoryId($value[1]);
+ $item->setTitle($value[2]);
+ $item->save();
+ $result[] = $item;
+ }
+
+ return $result;
+
+ }
+
+ public function testMultipleScopes()
@staabm

staabm Feb 26, 2013

Member

Requires tests for new method signatures like findbyRank etc.

@staabm staabm commented on the diff Feb 26, 2013

generator/lib/behavior/sortable/SortableBehavior.php
}
}
}
}
+ /**
+ * Generates the method argument signature, the appropriate phpDoc for @params,
+ * the scope builder php code and the scope variable builder php code/
+ *
+ * @return array ($methodSignature, $paramsDoc, $scopeBuilder, $buildScopeVars)
+ */
+ public function generateScopePhp()
+ {
+
@staabm

staabm Feb 26, 2013

Member

empty line at beginning of block

@staabm staabm commented on the diff Feb 26, 2013

generator/lib/behavior/sortable/SortableBehavior.php
+ /**
+ * Generates the method argument signature, the appropriate phpDoc for @params,
+ * the scope builder php code and the scope variable builder php code/
+ *
+ * @return array ($methodSignature, $paramsDoc, $scopeBuilder, $buildScopeVars)
+ */
+ public function generateScopePhp()
+ {
+
+ $methodSignature = '';
+ $paramsDoc = '';
+ $buildScope = '';
+ $buildScopeVars = '';
+
+ if ($this->hasMultipleScopes()) {
+
@staabm

staabm Feb 26, 2013

Member

empty line at beginning of block

@staabm staabm commented on the diff Feb 26, 2013

generator/lib/behavior/sortable/SortableBehavior.php
+ public function generateScopePhp()
+ {
+
+ $methodSignature = '';
+ $paramsDoc = '';
+ $buildScope = '';
+ $buildScopeVars = '';
+
+ if ($this->hasMultipleScopes()) {
+
+ $methodSignature = array();
+ $buildScope = array();
+ $paramsDoc = array();
+
+ foreach ($this->getScopes() as $idx => $scope) {
+
@staabm

staabm Feb 26, 2013

Member

empty line at beginning of block

@staabm staabm commented on the diff Feb 26, 2013

generator/lib/behavior/sortable/SortableBehavior.php
+ $buildScope[] = " \$scope[] = $param;\n";
+ $buildScopeVars[] = " $param = \$scope[$idx];\n";
+ $paramsDoc[] = " * @param ".$column->getPhpType()." $param Scope value for column `".$column->getPhpName()."`";
+
+ if (!$column->isNotNull()) {
+ $param .= ' = null';
+ }
+ $methodSignature[] = $param;
+ }
+
+ $methodSignature = implode(', ', $methodSignature);
+ $paramsDoc = implode("\n", $paramsDoc);
+ $buildScope = "\n".implode('', $buildScope)."\n";
+ $buildScopeVars = "\n".implode('', $buildScopeVars)."\n";
+
+ } else if ($this->useScope()){
@staabm

staabm Feb 26, 2013

Member

missing space

@staabm staabm commented on the diff Feb 26, 2013

...ior/sortable/SortableBehaviorQueryBuilderModifier.php
}
$script .= "
* @param PropelPDO \$con optional connection
*
* @return {$this->objectClassname}
*/
-public function findOneByRank(\$rank, " . ($useScope ? "\$scope = null, " : "") . "PropelPDO \$con = null)
-{
+public function findOneByRank(\$rank, " . ($useScope ? "$methodSignature, " : "") . "PropelPDO \$con = null)
@staabm

staabm Feb 26, 2013

Member

should the method verify the types of the given scope parameters and e.g. throw exception to assist the user while using the API?

Contributor

alanbem commented Mar 1, 2013

I cant think of a good solution
using several parameter tags...

Well, I can think of one

    <parameter name="scope" value="user_id" />
    <parameter name="scope" value="group_id" />

although I don't know how does it look like after xml to array conversion.

Member

jaugustin commented Mar 2, 2013

Multiple parameters won't work, the last one will overwrite the others.

@staabm staabm commented on the diff Mar 3, 2013

...ior/sortable/SortableBehaviorQueryBuilderModifier.php
*/
-public function inList(\$scope = null)
+public function inList($methodSignature)
@staabm

staabm Mar 3, 2013

Member

@marcj I played with the new sortable implementation with multi scopes...

when changing the schema to

    <table name="sortable_multi_scopes">
        <column name="id" required="true" primaryKey="true" autoIncrement="true" type="INTEGER" />
        <column name="category_id" required="true" type="INTEGER" />
        <column name="sub_category_id" type="INTEGER" />
        <column name="title" type="VARCHAR" size="100" primaryString="true" />
        <column name="position" type="INTEGER" />
        <behavior name="sortable">
            <parameter name="rank_column" value="position" />
            <parameter name="use_scope" value="true" />
            <parameter name="scope_column" value="sub_category_id, category_id" />
        </behavior>
    </table>

it generates methods like

    /**
     * Returns the objects in a certain list, from the list scope
     *
     * @param     int $scopeSubCategoryId Scope value for column `SubCategoryId`
     * @param     int $scopeCategoryId Scope value for column `CategoryId`
     *
     * @return SortableMultiScopesQuery The current query, for fluid interface
     */
    public function inList($scopeSubCategoryId = null, $scopeCategoryId)

here we have a problem, because the first arg is optional and the second is required.

2 possible solutions:
a) the behavior re-sorts the parameters from required to optional
b) we add a warning to the docs and tell the user that he needs to define the multi-scope parameter in a proper order

@marcj

marcj Mar 3, 2013

Owner

I don't see a problem here.

@staabm

staabm Mar 3, 2013

Member

ok, added a note about this in staabm/propelorm.github.com@e72e5dc .

Owner

willdurand commented Mar 4, 2013

Note that you can use multiple parameters with the same name. see the StateMachineBehavior: https://github.com/willdurand/StateMachineBehavior/blob/master/src/StateMachineBehavior.php#L57-L81

Member

staabm commented Mar 10, 2013

@marcj so should we use several scope_column parameters or should we stay with the comma separated approach?

Owner

willdurand commented Mar 25, 2013

I would go for several scope_column

Owner

marcj commented Apr 10, 2013

I implemented both ways now. ✌️
@staabm, could you please update the documentation?

Member

staabm commented Apr 10, 2013

will do the docs when @willdurand gives his ok having it both ways.

Owner

willdurand commented Apr 11, 2013

Is it mergeable now?

Member

staabm commented Apr 11, 2013

seems like it needs a rebase...

@willdurand are you ok with implementing the configuration in both ways (those are already implemented here by @marcj) ?

config with one coma separated scope_column tag:

+  <behavior name="sortable">
+    <parameter name="use_scope" value="true" />
+    <parameter name="scope_column" value="user_id, group_id" />
+  </behavior> 

config with several scope_column tags:

+  <behavior name="sortable">
+    <parameter name="use_scope" value="true" />
+    <parameter name="scope_column" value="user_id" />
+    <parameter name="scope_column" value="group_id" />
+  </behavior> 

?

Owner

willdurand commented Apr 11, 2013

I'm ok, as far as both are used the same way in the behavior itself, meaning it should just be syntactic sugar in the configuration file.

Member

staabm commented Apr 11, 2013

ok will adjust the doc accordingly. @marcj could you re-add a test for the comma separated form?

Owner

willdurand commented Apr 11, 2013

The PR needs a rebase, and a test for the comma separated form :)

@staabm staabm referenced this pull request in propelorm/propelorm.github.com Apr 15, 2013

Merged

bootstrapped sortable multi-column docs #212

Owner

marcj commented Apr 19, 2013

Added tests.

Member

staabm commented Apr 27, 2013

@willdurand this and the correspondings docs PR are ready 2 land

willdurand added a commit that referenced this pull request Apr 28, 2013

Merge pull request #616 from marcj/sortable-multi-scopes
Added multiple scope support in Sortable behavior.

@willdurand willdurand merged commit 4f33db4 into propelorm:master Apr 28, 2013

1 check passed

default The Travis build passed
Details
Owner

willdurand commented Apr 28, 2013

thanks!

willdurand added a commit to propelorm/Propel2 that referenced this pull request May 15, 2013

staabm added a commit to staabm/Propel2 that referenced this pull request May 16, 2013

staabm added a commit to staabm/Propel2 that referenced this pull request May 18, 2013

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