Doctrine 1
  1. Doctrine 1
  2. DC-659

Sluggable behavior does not check uniqueness on insert if a slug is manually set, causing SQL error/crash

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 1.2.2
    • Fix Version/s: None
    • Component/s: Behaviors
    • Labels:
      None
    • Environment:
      symfony-1.3.4 and doctrine-1.2.2

      Description

      The Sluggable behavior has the following code:

      Sluggable.php
          /**
           * Set the slug value automatically when a record is inserted
           *
           * @param Doctrine_Event $event
           * @return void
           */
          public function preInsert(Doctrine_Event $event)
          {
              $record = $event->getInvoker();
              $name = $record->getTable()->getFieldName($this->_options['name']);
      
              if ( ! $record->$name) {
                  $record->$name = $this->buildSlugFromFields($record);
              }
          }
      

      However, this can lead to problems...

      If the user incorrectly assigns a duplicate slug to the record then there is no uniqueness checking in doctrine and you get an uncaught SQL error looking something like this:

      SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'my-slug-en_GB' for key 'foo_i18n_sluggable_idx'
      

      If this kind of "don't do a preInsert check if I manunally set the slug" behavior is a FEATURE then it would be best to have an option to allow it to be disabled. If it is a BUG then I would suggest that the preInsert method should be changed to:

      Sluggable.php
          /**
           * Set the slug value automatically when a record is inserted
           *
           * @param Doctrine_Event $event
           * @return void
           */
          public function preInsert(Doctrine_Event $event)
          {
              $record = $event->getInvoker();
              $name = $record->getTable()->getFieldName($this->_options['name']);
      
              if ( ! $record->$name) {
                  $record->$name = $this->buildSlugFromFields($record);
              } else { // Still check for slug uniqueness when you insert
                  $record->$name = $this->buildSlugFromSlugField($record);
              }
          }
      

      C

        Activity

        Christian Seaman created issue -
        Christian Seaman made changes -
        Field Original Value New Value
        Description The Sluggable behavior has the following code:
        \\

        {code:title=Sluggable.php}
            /**
             * Set the slug value automatically when a record is inserted
             *
             * @param Doctrine_Event $event
             * @return void
             */
            public function preInsert(Doctrine_Event $event)
            {
                $record = $event->getInvoker();
                $name = $record->getTable()->getFieldName($this->_options['name']);

                if ( ! $record->$name) {
                    $record->$name = $this->buildSlugFromFields($record);
                }
            }
        {code}

        However, this can lead to problems...

        If the user incorrectly assigns a duplicate slug to the record then the {noformat} if ( ! $record->$name) {noformat} condition evaluates false and the buildSlugFromFields() method is not called.

        I am wondering why this is done and think that, perhaps, the if() check here does not make sense. Especially given that:
        # the preUpdate() method _always_ calls buildSlugFromFields() when $record->name is modified
        # if the (manually set) slug is already unique then buildSlugFromFields() will not change the value. However, if it clashes with something, then it will adapt it to enforce uniqueness.

        If this check is not done and you try to save a duplicate slug then you get an uncaught SQL error looking something like this
        {noformat}
        SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'my-slug-en_GB' for key 'foo_i18n_sluggable_idx'
        {noformat}

        If this kind of "don't do a preInsert check if I manunally set the slug" behavior is a _FEATURE_ then it would be best to have an option to allow it to be disabled. If it is a _BUG_ then I would suggest that the preInsert method should be changed to:

        {code:title=Sluggable.php}
            /**
             * Set the slug value automatically when a record is inserted
             *
             * @param Doctrine_Event $event
             * @return void
             */
            public function preInsert(Doctrine_Event $event)
            {
                $record = $event->getInvoker();
                $name = $record->getTable()->getFieldName($this->_options['name']);

                $record->$name = $this->buildSlugFromFields($record);
            }
        {code}


        C
        The Sluggable behavior has the following code:
        \\

        {code:title=Sluggable.php}
            /**
             * Set the slug value automatically when a record is inserted
             *
             * @param Doctrine_Event $event
             * @return void
             */
            public function preInsert(Doctrine_Event $event)
            {
                $record = $event->getInvoker();
                $name = $record->getTable()->getFieldName($this->_options['name']);

                if ( ! $record->$name) {
                    $record->$name = $this->buildSlugFromFields($record);
                }
            }
        {code}

        However, this can lead to problems...

        If the user incorrectly assigns a duplicate slug to the record then there is no uniqueness checking in doctrine and you get an uncaught SQL error looking something like this:
        {noformat}
        SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'my-slug-en_GB' for key 'foo_i18n_sluggable_idx'
        {noformat}

        If this kind of "don't do a preInsert check if I manunally set the slug" behavior is a _FEATURE_ then it would be best to have an option to allow it to be disabled. If it is a _BUG_ then I would suggest that the preInsert method should be changed to:

        {code:title=Sluggable.php}
            /**
             * Set the slug value automatically when a record is inserted
             *
             * @param Doctrine_Event $event
             * @return void
             */
            public function preInsert(Doctrine_Event $event)
            {
                $record = $event->getInvoker();
                $name = $record->getTable()->getFieldName($this->_options['name']);

                if ( ! $record->$name) {
                    $record->$name = $this->buildSlugFromFields($record);
                } else { // Still check for slug uniqueness when you insert
                    $record->$name = $this->buildSlugFromSlugField($record);
                }
            }
        {code}


        C

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Christian Seaman
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: