Doctrine 1
  1. Doctrine 1
  2. DC-64

Sluggable extension: new provider option for custom unique slugs

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2.0-ALPHA3
    • Component/s: Sluggable
    • Labels:
      None

      Description

      Recently I had a model where I needed a custom unique slug instead of a slug which is built from fields.

      At first I thought that maybe the "builder"-option was there for that purpose but soon I realized that this is obviously not the right one.

      The only left option was that I would need to create the method "getUniqueSlug" in the record class. This is of course an option and has its uses but with it you would have to reimplement the collision detection for uniqueness again which are already implemented in the getUniqueSlug-method of the sluggable class.

      After reading the code several hours I came to the conclusion that this functionality is simply missing so I created a new option called "provider" which is a callback. This callback provides the equivalent of the concatenation of the table fields. The great thing about this is that you don't have to care for uniqueness or the proper urlized format because this is still taken care of by the sluggable behaviour. Thus this patch is very unobtrusive and completely backwards-compatible of course.

      Usage is pretty straight-forward:

      model:
        actAs:
          Sluggable:
            provider: [modelTable, provideSlug]
      

      Or whatever you want to name the method which is called.

      jwage responded in trac:

      This is already possible if you just implement a toString() method and don't specify any fields.

      But I have to disagree. If I use toString() I still have to care about the uniqueness of the slug by myself because only when fields are specified this gets called inside the code:

      $this->getUniqueSlug($record, $value);
      

      Otherwise only the builder-function is called.

      So atm this is NOT possible.

      1. sluggable_listener.patch
        0.9 kB
        Maik Riechert
      2. sluggable.patch
        0.6 kB
        Maik Riechert

        Activity

        Hide
        Jonathan H. Wage added a comment -

        Ah Ok. I think maybe we could make some change to always run the slug through the function to ensure uniqueness and proper format. We could try that.

        Show
        Jonathan H. Wage added a comment - Ah Ok. I think maybe we could make some change to always run the slug through the function to ensure uniqueness and proper format. We could try that.
        Hide
        Maik Riechert added a comment -

        If I might say this I think that the behaviour of the sluggable template is highly incomprehensible if you don't use the standard function with simple fields. It looks like a bit of patchwork to me.

        The first thing I don't quite understand is that why is there an option to disable uniqueness? Isn't a slug by itself supposed to be always unique? What other uses could a slug have if it isn't?

        Second, I don't think that "$value = (string) $record;" should be anywhere used for real logic. The toString() method is actually there for debugging purposes but not for providing slugs or anything else.

        If you want to change it then really change it but not by doing a "$this->getUniqueSlug(..)" around the "(string) $record". I know that backwards compatibility is important. That is why I thought about a very unobtrusive way. Maybe if you look at the patch a second time you're starting to like it more

        Show
        Maik Riechert added a comment - If I might say this I think that the behaviour of the sluggable template is highly incomprehensible if you don't use the standard function with simple fields. It looks like a bit of patchwork to me. The first thing I don't quite understand is that why is there an option to disable uniqueness? Isn't a slug by itself supposed to be always unique? What other uses could a slug have if it isn't? Second, I don't think that "$value = (string) $record;" should be anywhere used for real logic. The toString() method is actually there for debugging purposes but not for providing slugs or anything else. If you want to change it then really change it but not by doing a "$this->getUniqueSlug(..)" around the "(string) $record". I know that backwards compatibility is important. That is why I thought about a very unobtrusive way. Maybe if you look at the patch a second time you're starting to like it more
        Hide
        Jonathan H. Wage added a comment -

        Fixed by r6496 in Doctrine 1.2

        Show
        Jonathan H. Wage added a comment - Fixed by r6496 in Doctrine 1.2
        Hide
        Maik Riechert added a comment -

        Thanks for implementing it. Although I don't use this possibility I think you broke backwards-compatibility which I wanted to avoid, notably for the getUniqueSlug-way because in the past this was the way for caring about the uniqueness on ones own, and now you make it unique again by doing the "if ($this->_options['unique'] === true)

        {" check for every way. This probably won't make a difference because it's unique in the first place but the point of having this "}

        else if (method_exists($record, 'getUniqueSlug')) {" was that one could save the effort that doctrine itself would have made which are also db queries. So effectively you lowered the performance of Sluggable in some cases

        Show
        Maik Riechert added a comment - Thanks for implementing it. Although I don't use this possibility I think you broke backwards-compatibility which I wanted to avoid, notably for the getUniqueSlug-way because in the past this was the way for caring about the uniqueness on ones own, and now you make it unique again by doing the "if ($this->_options ['unique'] === true) {" check for every way. This probably won't make a difference because it's unique in the first place but the point of having this "} else if (method_exists($record, 'getUniqueSlug')) {" was that one could save the effort that doctrine itself would have made which are also db queries. So effectively you lowered the performance of Sluggable in some cases
        Hide
        Maik Riechert added a comment -

        I have to correct myself. Of course you didn't break backwards compatibility with the getUniqueSlug-way, this will be only slower. The one where you broke it is where it gets the slug with "$value = (string) $record;" but this is indeed the right way to go because no one would suspect that a __toString method should care about uniqueness.

        My suggestion:

        Instead of

        if ($this->_options['unique'] === true)

        { return $this->getUniqueSlug($record, $value); }

        do

        if ($this->_options['unique'] === true && !method_exists($record, 'getUniqueSlug')) { return $this->getUniqueSlug($record, $value); }

        This should also make it quite clear to see that Sluggable always handles uniqueness if it's requested but for the getUniqueSlug-way. Maybe you should put the method_exists() in a variable to not have it called twice.

        Show
        Maik Riechert added a comment - I have to correct myself. Of course you didn't break backwards compatibility with the getUniqueSlug-way, this will be only slower. The one where you broke it is where it gets the slug with "$value = (string) $record;" but this is indeed the right way to go because no one would suspect that a __toString method should care about uniqueness. My suggestion: Instead of if ($this->_options ['unique'] === true) { return $this->getUniqueSlug($record, $value); } do if ($this->_options ['unique'] === true && !method_exists($record, 'getUniqueSlug')) { return $this->getUniqueSlug($record, $value); } This should also make it quite clear to see that Sluggable always handles uniqueness if it's requested but for the getUniqueSlug-way. Maybe you should put the method_exists() in a variable to not have it called twice.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: