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

Add support for scope_column in sluggable behavior #341

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

justinhilles commented Apr 23, 2012

added a config option to support using a scope column

@willdurand willdurand commented on an outdated diff Apr 23, 2012

generator/lib/behavior/sluggable/SluggableBehavior.php
@@ -43,6 +44,10 @@ public function modifyTable()
$unique = new Unique($this->getColumnForParameter('slug_column'));
$unique->setName($this->getTable()->getCommonName() . '_slug');
$unique->addColumn($this->getTable()->getColumn($this->getParameter('slug_column')));
+ if($this->getParameter('scope_column'))
+ {
@willdurand

willdurand Apr 23, 2012

Owner

this curly bracket should be on the same line than the if

Owner

willdurand commented Apr 23, 2012

Can you add a unit test to prove your improvement?

Contributor

justinhilles commented Apr 23, 2012

I'll give it a shot but this is something ive never done but need to start doing :-)

Owner

willdurand commented Apr 23, 2012

Read other tests, it's often useful :)
If you have questions, just ask here or on IRC (#propel on freenode.net)

Owner

willdurand commented Apr 24, 2012

Thanks for the CS fix. Anyway, the only chance you get this PR merged is to add few unit tests.
Propel 1.6 is bug fixes only. I can make exceptions when it makes sense, but without few unit tests, I can't understand the purpose of this PR ;)

Contributor

justinhilles commented Apr 24, 2012

Yah i hear yah man, i haven't had a chance to really look into how to do
that, i know its probabky simple, its just something ive never done. Do
you need it ASAP?

On Tue, Apr 24, 2012 at 3:28 PM, William Durand <
reply@reply.github.com

wrote:

Thanks for the CS fix. Anyway, the only chance you get this PR merged is
to add few unit tests.
Propel 1.6 is bug fixes only. I can make exceptions when it makes sense,
but without few unit tests, I can't understand the purpose of this PR ;)


Reply to this email directly or view it on GitHub:
#341 (comment)

Owner

willdurand commented Apr 25, 2012

Describe your use case, show me the code, and I will help you.

Contributor

justinhilles commented Apr 25, 2012

So use case is I have a multi-site setup with a unique category tree for
each site. Categories also have slugs.

So i save a root category of "Home" in site 1, saves fine, save root
category of home in site 2. Gives a unique key error on the slug column.

This fix addes the scope_column to the uniqe index in the behavior so
that you still of unique slugs in the site, but can have the same slug
in different sites.

Hope I explained that right. Is how i explained that basically how i
write a unit test off of it?

Thanks for your help

Justin

On 04/25/2012 05:08 AM, William Durand wrote:

Describe your use case, show me the code, and I will help you.


Reply to this email directly or view it on GitHub:
#341 (comment)

Owner

willdurand commented Apr 26, 2012

Ah so, it's quite easy.

First, add a table_with_scope table in the schema.xml related to the Sluggable tests. Don't forget to add the scope_column parameter.

Then, use the following tests:

<?php
// test/testsuite/generator/behavior/sluggable/SluggableBehaviorTest.php

public function testUniqueViolationWithoutScope()
{
    TableWithScopeQuery::create()->deleteAll();
    $t = new TableWithScope();
    $t->setTitle('Hello, World');
    $t->save();
    $this->assertEquals('hello-world', $t->getSlug());

    try {
        $t = new TableWithScope();
        $t->setTitle('Hello, World');
        $t->save();

        $this->fail('Exception expected');
    } catch (Exception $e) {
       $this->assertTrue(true, 'Exception successfully thrown'); 
   }
}

public function testNoUniqueViolationWithScope()
{
    TableWithScope::create()->deleteAll();
    $t = new TableWithScope();
    $t->setTitle('Hello, World');
    $t->save();
    $this->assertEquals('hello-world', $t->getSlug());

    try {
        $t = new TableWithScope();
        $t->setTitle('Hello, World');
        $t->setScope('foo');
        $t->save();

        $this->assertEquals('hello-world', $t->getSlug());
    } catch (Exception $e) {
        $this->fail('Unexpected exception catched');
   }
}

My question is: what will be the behavior of the findOneBySlug() method?

Contributor

justinhilles commented Apr 26, 2012

I didnt see any code, just php tags, is that on purpose?

On Thu, Apr 26, 2012 at 12:11 AM, William Durand <
reply@reply.github.com

wrote:

Ah so, it's quite easy:

<?php

//

---
Reply to this email directly or view it on GitHub:
https://github.com/propelorm/Propel/pull/341#issuecomment-5350826
Owner

willdurand commented Apr 26, 2012

Go to GitHub

2012/4/26 Justin <
reply@reply.github.com

I didnt see any code, just php tags, is that on purpose?

On Thu, Apr 26, 2012 at 12:11 AM, William Durand <
reply@reply.github.com

wrote:

Ah so, it's quite easy:

<?php

//

---
Reply to this email directly or view it on GitHub:
https://github.com/propelorm/Propel/pull/341#issuecomment-5350826

Reply to this email directly or view it on GitHub:
#341 (comment)

Contributor

justinhilles commented Apr 26, 2012

Got it, wow thanks! I'll give this a try

Contributor

justinhilles commented Jun 4, 2012

Finally added tests for this. Let me know if they are correct, they are passing for me

Owner

willdurand commented Jun 5, 2012

@justinhilles awesome! Can you rebase your PR, then I'll merge it?

Contributor

justinhilles commented Jun 5, 2012

I forked the propel repo and was doing work in my version. I see its different than you explain here: http://www.propelorm.org/contribute.html. Should I do this over and start with that then add my changes? Or is there some slicker way?

justinhilles and others added some commits Apr 23, 2012

@justinhilles justinhilles + Justin Hilles added support for scope_column d3c281f
@justinhilles justinhilles + Justin Hilles tweak for proper php format def881b
@justinhilles justinhilles + Justin Hilles add test for sluggable with scope and alter make unique in behavior t…
…o include scope
834c0a4
Justin Hilles Merge remote-tracking branch 'origin/master'
Conflicts:
	test/testsuite/generator/behavior/sluggable/SluggableBehaviorTest.php
9ee3eb9

This pull request passes (merged 9ee3eb9 into 3b15493).

Contributor

justinhilles commented Jun 7, 2012

I think i rebased but it says in the last commit:
Conflicts:
test/testsuite/generator/behavior/sluggable/SluggableBehaviorTest.php

but i edited the conflict before i committed? Any idea if thats an issue?

This pull request passes (merged cb4acf5 into 3b15493).

Owner

willdurand commented Jun 11, 2012

Merged, thank you @justinhilles!

@willdurand willdurand closed this Jun 11, 2012

Owner

willdurand commented Jun 11, 2012

Could you port your PR on Propel2 as well? propelorm/Propel2#244

Cheers!

@willdurand willdurand referenced this pull request in propelorm/Propel2 Jul 24, 2012

Closed

Port PR #341 from Propel 1.6 #244

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