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

        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
        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?

          People

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

            Dates

            • Created:
              Updated: