[DC-728] Updating slug in I18N behaviour yields non-unique slug Created: 11/Jun/10  Updated: 05/Oct/10

Status: Open
Project: Doctrine 1
Component/s: I18n, Sluggable
Affects Version/s: 1.2.0, 1.2.1, 1.2.2
Fix Version/s: None

Type: Bug Priority: Major
Reporter: Florian Aschenbrenner Assignee: Jonathan H. Wage
Resolution: Unresolved Votes: 5
Labels: None
Environment:

Symfony 1.4.5
Doctrine ORM



 Description   

There seems to be a bug in the way, doctrine tries to find a unique slug for a table that is I18N-enabled and has a slug in it.

Following example:

Problem:
  actAs:
    I18n:
      fields: [name]
      actAs:
        Sluggable: { fields: [name], uniqueBy: [lang, name], canUpdate: true }
  columns:
    name: { type: string(255), notnull: true }

Now, if we insert a new entry, the slug get's created as expected. If we now insert another entry that would yield the same slug, the slug is made unique by adding the postfix as expected - all well so far.

Now, if we try to update the entry with the slug with the postfix, what i think is an error happens (Excerpt from Sluggable.php, lib/Doctrine/Template/Listener/Sluggable.php):

 
public function getUniqueSlug($record, $slugFromFields)
...
if ($record->exists()) {
            $identifier = $record->identifier();
            $whereString .= ' AND r.' . implode(' != ? AND r.', $table->getIdentifierColumnNames()) . ' != ?';
            $whereParams = array_merge($whereParams, array_values($identifier));
        }

        foreach ($this->_options['uniqueBy'] as $uniqueBy) {
            if (is_null($record->$uniqueBy)) {
                $whereString .= ' AND r.'.$uniqueBy.' IS NULL';
            } else {
                $whereString .= ' AND r.'.$uniqueBy.' = ?';
                $value = $record->$uniqueBy;
                if ($value instanceof Doctrine_Record) {
                    $value = current((array) $value->identifier());
                }
                $whereParams[] = $value;
            }
        }
...

So, the $record->exists() check evaluates to true and $table->getIdentifierColumnNames() yields the fields id and lang, so the whereString is something like

 
AND r.id != ? AND r.lang != ?

Now for the problematic part:
uniqueBy is lang and name; so the code adds to the whereString:

 
AND r.lang = ? AND r.name = ?

So the resulting whereString has something like

 
r.lang != ? AND r.lang = ?

which will never yield a result in my eyes, thus, the postfix is never incremented and the slug defaults to the the slug of name without postfix.



 Comments   
Comment by Rodrigo Borrego Bernabé [ 24/Jun/10 ]

Hi,

We've also found this problem and make a temporal workaround. But maybe our workaround can give someone an idea on how to fix the bug so I'm pasting the changed code here

In file sfDoctrinePlugin\lib\vendor\doctrine\Doctrine\Template\Listener\Sluggable.php (getUniqueSlug method)

Replace

if ($record->exists()) {
  $identifier = $record->identifier();
  $whereString .= ' AND r.' . implode(' != ? AND r.', $table->getIdentifierColumnNames()) . ' != ?';
  $whereParams = array_merge($whereParams, array_values($identifier));
}

With

if ($record->exists()) {
  $identifier = $record->identifier();
  $commonFields = array_uintersect($table->getIdentifierColumnNames(), $this->_options['uniqueBy'], "strcasecmp");
  foreach ($commonFields as $key => $value)
    unset ($identifier[$value]);
  
  $whereFields = array_diff($table->getIdentifierColumnNames(), $this->_options['uniqueBy']);
  $whereString .= ' AND r.' . implode(' != ? AND r.', $whereFields) . ' != ?';
  $whereParams = array_merge($whereParams, array_values($identifier));
}
Comment by Christian Seaman [ 05/Oct/10 ]

Rodrigo, I think the solution is simpler than you might expect...

The problem is that it is doing an AND where it should be doing an OR to find records other than the current one.

So, you just need to replace this part of Sluggable::getUniqueSlug():

Sluggable::getUniqueSlug()
...
        if ($record->exists()) {
            $identifier = $record->identifier();
            $whereString .= ' AND r.' . implode(' != ? AND r.', $table->getIdentifierColumnNames()) . ' != ?';
            $whereParams = array_merge($whereParams, array_values($identifier));
        }
...

With this code (the 3rd line has extra brackets and OR instead of AND):

Sluggable::getUniqueSlug() (modified)
        if ($record->exists()) {
            $identifier = $record->identifier();
            $whereString .= ' AND (r.' . implode(' != ? OR r.', $table->getIdentifierColumnNames()) . ' != ?)';
            $whereParams = array_merge($whereParams, array_values($identifier));
        }

In more detail, the WHERE clause being built up is saying:
"find me records WHERE (the slug begins with the same string) AND (it's not the same record) AND (the other key uniqueBy values are the same)"

However, to check that it's not the same record you just need one of the key values to be different (not all of them) so the check should be something like:

... AND (p.id != '2' OR p.lang != 'en') ...

instead of:

... AND p.id != '2' AND p.lang != 'en' ...

The modification to the code as stated above generates queries that look like this, which I'm pretty sure is correct:

... WHERE (p.url LIKE 'foo%' AND (p.id != '2' OR p.lang != 'en') AND p.lang = 'en' AND p.other_uniqueby_field = '1')

C

Generated at Mon Oct 20 21:41:17 UTC 2014 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.