Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Too many queries for the makeSlugUnique method #447

Closed
wants to merge 7 commits into from

4 participants

@Divi

There is a problem with the method makeSlugUnique, indeed, when you choose just one column to sluggable and if this column can be exists many time in the database, the makeSlugUnique will be launched as many times that you have a line with the same column value.

For exemple, if you have a column "label", and a categoy with label value : "Home", now try to insert new row 100 times with the label value "Home", the makeUniqueSlug with be launched n+1 times at each row.

So, I changed the method : try one time without ending separator, and if the line already exists, I try one more time but with the ending sepator and the count + 1 times of the line exists. So two queries max per line.

Divi added some commits
@Divi Divi Too many queries for the makeSlugUnique method
There is a problem with the method makeSlugUnique, indeed, when you choose just one column to sluggable and if this column can be exists many time in the database, the makeSlugUnique will be launched as many times that you have a line with the same column value.

For exemple, if you have a column "label", and a categoy with label value : "Home", now try to insert new row 100 times with the label value "Home", the makeUniqueSlug with be launched n+1 times at each row.

So, I changed the method : try one time without ending separator, and if the line already exists, I try one more time but with the ending sepator and the count + 1 times of the line exists. So two queries max per line.
5348b23
@Divi Divi Miss PHP Doc' dc31f15
@travisbot

This pull request fails (merged dc31f15 into 441f969).

@travisbot

This pull request passes (merged 39a5bee into 441f969).

@travisbot

This pull request fails (merged b9cdc5c into 441f969).

@travisbot

This pull request fails (merged eb5921f into 441f969).

@travisbot

This pull request fails (merged 226e460 into 441f969).

@Divi Divi Fix object without pattern
Sorry, can't launch test myself atm.
ac39007
@travisbot

This pull request passes (merged ac39007 into 441f969).

@jaugustin jaugustin commented on the diff
generator/lib/behavior/sluggable/SluggableBehavior.php
((9 lines not shown))
+ $script = "
+if ((\$this->isColumnModified($const) || ";
+ $count = preg_match_all('{[a-zA-Z]+}', $pattern, $matches, PREG_PATTERN_ORDER);
+
+ foreach ($matches[0] as $key => $match) {
+ $column = $this->getTable()->getColumn($this->underscore($match));
+ if (null == $column) {
+ throw new \InvalidArgumentException('The pattern ' . $match . ' is invalid ! Please use PASCAL naming.');
+ }
+ $columnConst = $builder->getColumnConstant($column);
+ $script .= "\$this->isColumnModified($columnConst)" . ($key < $count - 1 ? " || " : "");
+ }
+
+ $script .= ") && \$this->{$this->getColumnGetter()}()) {";
+ }
+ else {
@jaugustin Collaborator

closing bracket should be on the same line as else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin jaugustin commented on the diff
generator/lib/behavior/sluggable/SluggableBehavior.php
((37 lines not shown))
\$this->{$this->getColumnSetter()}(\$this->createSlug());
}";
- } else {
- $script .= "
-} else {
+ }
+ else {
@jaugustin Collaborator

idem for closing bracket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaugustin
Collaborator

is it possible to add tests to prove the fix ?
and also add tests for the new function underscore

@willdurand
Owner

ping @Divi

@Divi

I'll make tests soon (maybe end of this month), but for now, I've no time sorry.

@willdurand willdurand closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 14, 2012
  1. @Divi

    Too many queries for the makeSlugUnique method

    Divi authored
    There is a problem with the method makeSlugUnique, indeed, when you choose just one column to sluggable and if this column can be exists many time in the database, the makeSlugUnique will be launched as many times that you have a line with the same column value.
    
    For exemple, if you have a column "label", and a categoy with label value : "Home", now try to insert new row 100 times with the label value "Home", the makeUniqueSlug with be launched n+1 times at each row.
    
    So, I changed the method : try one time without ending separator, and if the line already exists, I try one more time but with the ending sepator and the count + 1 times of the line exists. So two queries max per line.
  2. @Divi

    Miss PHP Doc'

    Divi authored
  3. @Divi

    Fix LIKE condition

    Divi authored
Commits on Aug 16, 2012
  1. @Divi
  2. @Divi

    Fix method name

    Divi authored
  3. @Divi

    Oops... fix count query

    Divi authored
  4. @Divi

    Fix object without pattern

    Divi authored
    Sorry, can't launch test myself atm.
This page is out of date. Refresh to see the latest.
Showing with 77 additions and 20 deletions.
  1. +77 −20 generator/lib/behavior/sluggable/SluggableBehavior.php
View
97 generator/lib/behavior/sluggable/SluggableBehavior.php
@@ -79,21 +79,43 @@ protected function getColumnSetter()
public function preSave($builder)
{
$const = $builder->getColumnConstant($this->getColumnForParameter('slug_column'));
- $script = "
-if (\$this->isColumnModified($const) && \$this->{$this->getColumnGetter()}()) {
+ $pattern = $this->getParameter('slug_pattern');
+
+ if ($pattern && $this->getParameter('permanent') != 'true') {
+ $script = "
+if ((\$this->isColumnModified($const) || ";
+ $count = preg_match_all('{[a-zA-Z]+}', $pattern, $matches, PREG_PATTERN_ORDER);
+
+ foreach ($matches[0] as $key => $match) {
+ $column = $this->getTable()->getColumn($this->underscore($match));
+ if (null == $column) {
+ throw new \InvalidArgumentException('The pattern ' . $match . ' is invalid ! Please use PASCAL naming.');
+ }
+ $columnConst = $builder->getColumnConstant($column);
+ $script .= "\$this->isColumnModified($columnConst)" . ($key < $count - 1 ? " || " : "");
+ }
+
+ $script .= ") && \$this->{$this->getColumnGetter()}()) {";
+ }
+ else {
@jaugustin Collaborator

closing bracket should be on the same line as else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $script .= "if (\$this->isColumnModified($const) && \$this->{$this->getColumnGetter()}()) {";
+ }
+
+ $script .= "
\$this->{$this->getColumnSetter()}(\$this->makeSlugUnique(\$this->{$this->getColumnGetter()}()));";
- if ($this->getParameter('permanent') == 'true') {
- $script .= "
-} elseif (!\$this->{$this->getColumnGetter()}()) {
+
+ if (null == $pattern && $this->getParameter('permanent') != 'true') {
+ $script .= "
+} else {
\$this->{$this->getColumnSetter()}(\$this->createSlug());
}";
- } else {
- $script .= "
-} else {
+ }
+ else {
@jaugustin Collaborator

idem for closing bracket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $script .= "
+} elseif (!\$this->{$this->getColumnGetter()}()) {
\$this->{$this->getColumnSetter()}(\$this->createSlug());
}";
- }
-
+ }
return $script;
}
@@ -265,14 +287,36 @@ public function addMakeSlugUnique(&$script)
*
* @param string \$slug the slug to check
* @param string \$separator the separator used by slug
- * @param int \$increment the count of occurences of the slug
+ * @param int \$alreadyExists false for the first try, true for the second, and take the high count + 1
* @return string the unique slug
*/
-protected function makeSlugUnique(\$slug, \$separator = '" . $this->getParameter('separator') ."', \$increment = 0)
+protected function makeSlugUnique(\$slug, \$separator = '" . $this->getParameter('separator') ."', \$alreadyExists = false)
{
- \$slug2 = empty(\$increment) ? \$slug : \$slug . \$separator . \$increment;
- \$slugAlreadyExists = " . $this->builder->getStubQueryBuilder()->getClassname() . "::create()
- ->filterBySlug(\$slug2)
+ if (!\$alreadyExists) {
+ \$slug2 = \$slug;
+ }
+ else {
+ \$slug2 = \$slug . \$separator;";
+
+ if (null == $this->getParameter('slug_pattern')) {
+ $getter = $this->getColumnGetter();
+ $script .= "
+
+ \$count = " . $this->builder->getStubQueryBuilder()->getClassname() . "::create()
+ ->filterBySlug(\$this->$getter())
+ ->filterByPrimaryKey(\$this->getPrimaryKey())
+ ->count();
+
+ if (1 == \$count) {
+ return \$this->$getter();
+ }";
+ }
+
+ $script .= "
+ }
+
+ \$count = " . $this->builder->getStubQueryBuilder()->getClassname() . "::create()
+ ->filterBySlug(\$alreadyExists ? \$slug2 . '%' : \$slug2)
->prune(\$this)";
if ($this->getParameter('scope_column')) {
@@ -286,12 +330,16 @@ protected function makeSlugUnique(\$slug, \$separator = '" . $this->getParameter
->includeDeleted()";
}
$script .= "
- ->count();
- if (\$slugAlreadyExists) {
- return \$this->makeSlugUnique(\$slug, \$separator, ++\$increment);
- } else {
+ ->count();
+
+ if (!\$alreadyExists && \$count > 0) {
+ return \$this->makeSlugUnique(\$slug, \$separator, true);
+ }
+ elseif (!\$alreadyExists && \$count == 0) {
return \$slug2;
}
+
+ return \$slug2 . (\$count + 1);
}
";
}
@@ -343,4 +391,13 @@ public function findOneBySlug(\$slug, \$con = null)
";
}
-}
+ /**
+ * @param string $string
+ *
+ * @return string
+ */
+ protected function underscore($string)
+ {
+ return strtolower(preg_replace(array('/([A-Z]+)([A-Z][a-z])/', '/([a-z\d])([A-Z])/'), array('\\1_\\2', '\\1_\\2'), strtr($string, '_', '.')));
+ }
+}
Something went wrong with that request. Please try again.