[#DC-728] Updating slug in I18N behaviour yields non-unique slug - Doctrine Project

Details

Type: Bug
Status: Open
Priority: Major
Resolution: Unresolved
Affects Version/s: 1.2.0, 1.2.1, 1.2.2
Fix Version/s: None
Component/s: I18n, Sluggable
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.

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Rodrigo Borrego Bernabé added a comment - 24/Jun/10 1:11 PM

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));
}

Show

Rodrigo Borrego Bernabé added a comment - 24/Jun/10 1:11 PM 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)); }

Hide

Permalink

Christian Seaman added a comment - 05/Oct/10 11:28 AM

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

Show

Christian Seaman added a comment - 05/Oct/10 11:28 AM 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

Updating slug in I18N behaviour yields non-unique slug

Details

Description

Activity

People

Dates