Doctrine 1

Sluggable canUpdate overridden after subsequent updates

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 1.2.0, 1.2.1, 1.2.2, 1.2.3, 1.2.4
  • Fix Version/s: None
  • Component/s: Behaviors
  • Labels:
    None
  • Environment:
    WAMP stack - PHP 5.3

Description

When allowing the user to manually change a slug using 'canUpdate' the slug reverts back to it's default generated value upon subsequent saves. Pre-Update on the Sluggable Listener Template seems to incorrectly decide to regenerate the default value.

Example:

$this->actAs('Sluggable', array('unique'=>true, 'fields'=>array('title'), 'canUpdate'=>true));

$record->description = "An example Item";
$record->title = "Example Title";
$record->save();

echo $record->slug; //"example-title" (Correct)

$record->description = "An example Item";
$record->title = "Example Title";
$record->slug = "custom-slug";
$record->save();

echo $record->slug; //"custom-slug" (Correct - First Save)

$record->description = "An example Item";
$record->title = "Example Title";
$record->slug = "custom-slug";
$record->save();
echo $record->slug; //"example-title" (Incorrect - Subsequent Save. Has regenerated it's default slug and lost the user defined one - even though we have passed the users custom one to the object prior to saving it.).

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Christian Seaman added a comment -

As far as I can see this is a problem with the logic in Sluggable::preUpdate() , I recently posted a comment about this here:
http://groups.google.com/group/doctrine-user/browse_thread/thread/d40c6ac733738d4a

In short, I think that the code should be changed from:

Sluggable.php
    public function preUpdate(Doctrine_Event $event)
    {
        if (false !== $this->_options['unique']) {
            $record = $event->getInvoker();
            $name = $record->getTable()->getFieldName($this->_options['name']);

            if ( ! $record->$name || (
                false !== $this->_options['canUpdate'] &&
                ! array_key_exists($name, $record->getModified())
            )) {
                $record->$name = $this->buildSlugFromFields($record);
            } else if ( ! empty($record->$name) &&
                false !== $this->_options['canUpdate'] &&
                array_key_exists($name, $record->getModified()
            )) {
                $record->$name = $this->buildSlugFromSlugField($record);
            }
        }
    }

To this (i.e. remove the canUpdate conditions from the first inner if):

Sluggable.php (modified)
    public function preUpdate(Doctrine_Event $event)
    {
        if (false !== $this->_options['unique']) {
            $record = $event->getInvoker();
            $name = $record->getTable()->getFieldName($this->_options['name']);

            if ( ! $record->$name) { // i.e. remove the other conditions - you should only build the slug from other fields if it's empty
                $record->$name = $this->buildSlugFromFields($record);
            // possibly add an "else if !canUpdate then make sure the old value is preserved" here
            } else if ( ! empty($record->$name) &&
                false !== $this->_options['canUpdate'] &&
                array_key_exists($name, $record->getModified()
            )) {
                $record->$name = $this->buildSlugFromSlugField($record);
            }
        }
    }

I have modified my local version of the Doctrine code to use this modification and it seems to (a) not have the problem you reported above and (b) generally work ok. However, I have not run the test suite against it.

C

Show
Christian Seaman added a comment - As far as I can see this is a problem with the logic in Sluggable::preUpdate() , I recently posted a comment about this here: http://groups.google.com/group/doctrine-user/browse_thread/thread/d40c6ac733738d4a In short, I think that the code should be changed from: Sluggable.php public function preUpdate(Doctrine_Event $event) { if ( false !== $ this ->_options['unique']) { $record = $event->getInvoker(); $name = $record->getTable()->getFieldName($ this ->_options['name']); if ( ! $record->$name || ( false !== $ this ->_options['canUpdate'] && ! array_key_exists($name, $record->getModified()) )) { $record->$name = $ this ->buildSlugFromFields($record); } else if ( ! empty($record->$name) && false !== $ this ->_options['canUpdate'] && array_key_exists($name, $record->getModified() )) { $record->$name = $ this ->buildSlugFromSlugField($record); } } } To this (i.e. remove the canUpdate conditions from the first inner if): Sluggable.php (modified) public function preUpdate(Doctrine_Event $event) { if ( false !== $ this ->_options['unique']) { $record = $event->getInvoker(); $name = $record->getTable()->getFieldName($ this ->_options['name']); if ( ! $record->$name) { // i.e. remove the other conditions - you should only build the slug from other fields if it's empty $record->$name = $ this ->buildSlugFromFields($record); // possibly add an " else if !canUpdate then make sure the old value is preserved" here } else if ( ! empty($record->$name) && false !== $ this ->_options['canUpdate'] && array_key_exists($name, $record->getModified() )) { $record->$name = $ this ->buildSlugFromSlugField($record); } } } I have modified my local version of the Doctrine code to use this modification and it seems to (a) not have the problem you reported above and (b) generally work ok. However, I have not run the test suite against it. C
Hide
Permalink
Adam Benson added a comment -

Updated affected versions

Show
Adam Benson added a comment - Updated affected versions
Hide
Permalink
Adam Benson added a comment -

Thanks for the update Christian, perhaps you could share your fix as a patch?

Show
Adam Benson added a comment - Thanks for the update Christian, perhaps you could share your fix as a patch?
Hide
Permalink
Jean-Sébastien GERARD added a comment -

Hi, I'm working on a multilingue web portal with Symfony 1.4 and Doctrine 1.2.3, and man, your fix saves my life
I use Sluggable slaved by i18n and without your fix Sluggable simply does not do the job, instead it messes up all slugs when updating, eventually you can't retrieve object anymore based on it. I would not set it as only minor bug ?
thanks, anyway.

Show
Jean-Sébastien GERARD added a comment - Hi, I'm working on a multilingue web portal with Symfony 1.4 and Doctrine 1.2.3, and man, your fix saves my life I use Sluggable slaved by i18n and without your fix Sluggable simply does not do the job, instead it messes up all slugs when updating, eventually you can't retrieve object anymore based on it. I would not set it as only minor bug ? thanks, anyway.

People

  • Assignee:
    Jonathan H. Wage
    Reporter:
    Adam Benson
Vote (1)
Watch (2)

Dates

  • Created:
    Updated: