milesj / utility

[Deprecated] A CakePHP plugin for common utility classes.
MIT License
69 stars 24 forks source link

Sluggable _makeUnique is buggy #21

Closed dilab closed 10 years ago

dilab commented 10 years ago

_makeUnique functions checks the slug and append a count. This is not a correct way to make it unique.

e.g.

  1. save record 'abc', it generates slug 'abc'.
  2. save record 'abc', it generates slug 'abc-1'.
  3. delete record with slug 'abc'.
  4. save record 'abc', it generates slug 'abc-1'. BUG: now we have two records with slug 'abc-1'.
milesj commented 10 years ago

How so? Number 4 wouldn't generate "abc-1" as "abc" doesn't exist, it would stay as "abc".

dilab commented 10 years ago

Because you are using LIKE to check the slug.

So the conditions is [slug LIKE] => 'abc%'. Which will return count as 1.

That is how you get 'abc-1' at the Number 4.

Hope it is clear.

milesj commented 10 years ago

I'll have to change the LIKE logic then, because I guess "abcd" will also trigger it.

milesj commented 10 years ago

Rewrote the queries.

dilab commented 10 years ago

This will not work.

For example:

  1. abc -> abc
  2. abc -> abc-1($count=1)
  3. abc -> abc-1($conditions['slug']='abc', $count=1 && $conditions['slug'] = 'LIKE abc-%', $count=1)

You will see point 2 and 3 are producing the same slug.

Why do not you just use a while loop to check the uniqueness of the slug?

milesj commented 10 years ago

It runs 2 queries, so if our slug was abc.

1st query - If abc does not exist, then abc is the slug, else... 2nd query - Do a wildcard search for abc-%, which will return slug as abc-#.

The queries aren't doing an &&, they are 2 separate queries.

dilab commented 10 years ago

My point 3 is saying:

1st query: abc exists, and $count = 1 , so continue to 2nd query 2nd query: it only check abc-%, which will return $count = 1 again. Which generates a slug with value of 'abc-1'.

You see the problem here? point 2 and 3 are producing the same slug.

milesj commented 10 years ago

I just need to fix that if statement, one sec.