contao / core-bundle

[READ-ONLY] Contao Core Bundle
GNU Lesser General Public License v3.0
123 stars 58 forks source link

Service for generating alias #1671

Closed ausi closed 5 years ago

ausi commented 6 years ago

Related: #1598

As @Toflar noted in https://github.com/contao/core-bundle/commit/7d2daad52af73471ba717336529f2b601fd1d8eb#r30056164 we should probably create a service for generating aliases. IMO this could look like so:

public function generateSlug(string $text, iterable $options, callable $duplicateCheck = null): string
{
    $text = $this->framework->getAdapter(StringUtil::class)->prepareSlug($text);
    $slug = $this->slugGenerator->generate($text, $options);

    if (preg_match('/^[1-9][0-9]*$/', $slug)) {
        $slug = 'id-'.$slug;
    }

    if (null === $duplicateCheck) {
        return $slug;
    }

    $base = $slug;
    for ($count = 2; $duplicateCheck($slug); $count++) {
        $slug = $base.'-'.$count;
    }

    return $slug;
}

and can be used like so:

…->get('contao.?')->generateSlug(
    $dc->activeRecord->title, 
    $slugOptions,
    function($alias) {
        $result = $this->Database->prepare("SELECT id FROM tl_page WHERE alias=?")->execute($alias);
        return $result->numRows > 0;
    }
)

Does this make sense?
How should we name this service?

(In this context we should IMO also change the is_numeric() checks to a RegEx check `/^[1-9][0-9]$/`)*

Toflar commented 6 years ago

You don't need to get the adapter for the StringUtil class. It's not wrong to use a class statically as long as it has no external dependencies. So StringUtil::prepareSlug($text) is perfectly fine. And the rest of your proposal looks good to me 👍

aschempp commented 6 years ago

Are we generating the slug from multiple places? Or is this just used to generate the slug for a page alias? Then maybe we should simply implement that in an event listener instead of a generic slug service?

ausi commented 5 years ago

Closed in favor of contao/contao#222