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
        Hide
        Jonathan H. Wage added a comment -

        Can you provide your changes as a patch/diff with a test case?

        Show
        Jonathan H. Wage added a comment - Can you provide your changes as a patch/diff with a test case?
        Hide
        Christian Seaman added a comment -

        Hi Jonathan,

        I'm not so hot at making patches or test cases, but it should be fairly easy if you know what you're doing...

        Just try to create and save two records with the same hard-coded slug and the second one will fail with an ugly MySQL crash.

        If you add the lines

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

        to the Sluggable::preInsert() method then this problem is averted and the test cases will pass.

        I have been running with this modification in our production version of Doctrine since I first reported this in May and it all seems to work well.

        If you really need me to figure out how to make a patch and test case please re-comment on this ticket and I'll see what I can do when I have some free time.

        C

        Show
        Christian Seaman added a comment - Hi Jonathan, I'm not so hot at making patches or test cases, but it should be fairly easy if you know what you're doing... Just try to create and save two records with the same hard-coded slug and the second one will fail with an ugly MySQL crash. If you add the lines } else { // Still check for slug uniqueness when you insert $record->$name = $this->buildSlugFromSlugField($record); to the Sluggable::preInsert() method then this problem is averted and the test cases will pass. I have been running with this modification in our production version of Doctrine since I first reported this in May and it all seems to work well. If you really need me to figure out how to make a patch and test case please re-comment on this ticket and I'll see what I can do when I have some free time. C

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DC-659, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

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

            Dates

            • Created:
              Updated: