Doctrine 1
  1. Doctrine 1
  2. DC-746

Sluggable canUpdate overridden after subsequent updates

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.2.0, 1.2.1, 1.2.2, 1.2.3
    • 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

        Hide
        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
        Adam Benson added a comment -

        Updated affected versions

        Show
        Adam Benson added a comment - Updated affected versions
        Hide
        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
        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
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated: