Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auto generated alias converts some special characters to numbers #1334

Closed
fritzmg opened this issue Jan 22, 2018 · 15 comments
Closed

auto generated alias converts some special characters to numbers #1334

fritzmg opened this issue Jan 22, 2018 · 15 comments
Assignees
Labels
Milestone

Comments

@fritzmg
Copy link
Contributor

fritzmg commented Jan 22, 2018

Reproduction:

  1. Log into the Contao Online Demo.
  2. Create a new form in the form generator.
  3. Set the alias to Newsletter Anmeldung (de).
  4. Click save.

The auto generated alias will be newsletter-anmeldung-40-de-41.

Note: only some characters will actually be converted. For instance, the alias !"§$%&/()=? will be converted to 40-41-61.

@ausi
Copy link
Member

ausi commented Jan 22, 2018

Could you please check if replacing line

$varValue = System::getContainer()->get('contao.slug.generator')->generate(StringUtil::stripInsertTags($dc->activeRecord->title), $slugOptions);
with the following code solves the issue?

$varValue = System::getContainer()->get('contao.slug.generator')->generate(html_entity_decode(StringUtil::stripInsertTags($dc->activeRecord->title)), $slugOptions);

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 22, 2018

Yes, that solves the issue.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 22, 2018

However, shouldn't something like StringUtil::generateAlias be reintroduced, so that you don't have to do

html_entity_decode(StringUtil::stripInsertTags(…))

in every generateAlias function?

The old StringUtil::generateAlias function used to do:

  1. StringUtil::decodeEntities
  2. StringUtil::restoreBasicEntities
  3. standardize
    1. html_entity_decode
    2. strip_insert_tags
    3. utf8_romanize
    4. preg_replace

@ausi
Copy link
Member

ausi commented Jan 22, 2018

The correct order should be:

  1. strip insert tags
  2. restore basic entities
  3. decode entities
  4. generate slug

As suggested in #1016 (comment) should we an optional parameter to StringUtil::generateAlias() for the PageModel? So that we only have to call StringUtil::generateAlias($dc->activeRecord->title, $objPage)

@m-vo
Copy link
Member

m-vo commented Jan 22, 2018

Can't we put that into a service? This would imho make it easier for extensions to adapt.

@ausi
Copy link
Member

ausi commented Jan 22, 2018

How should it be called contao.slug.helper, contao.slug.alias_generator,...?

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 22, 2018

Well, why not contao.slug.generator? I know that service name is currently Ausi\SlugGenerator\SlugGenerator, but it could be something like Contao\CoreBundle\Slug\Generator instead, which in turn uses Ausi\SlugGenerator\SlugGenerator. Seems superfluous to register two services for that when one won't be used anymore (will it?)

@backbone87
Copy link
Contributor

backbone87 commented Jan 23, 2018

if we introduce a contao specific class we should stick to contao terminology: AliasGenerator (which internally uses @ausi's slug generator)
the service names i would use are: contao.alias_generator and contao.slug_generator
edit: having a registered service of type SlugGenerator maybe isnt needed at all. the AliasGenerator can privately create an instance and passthrou the configuration.

@ausi
Copy link
Member

ausi commented Jan 23, 2018

We cannot remove the contao.slug.generator service, this would break backwards compatibility.

Seems superfluous to register two services for that when one won't be used anymore (will it?)

Why would the current service not be used anymore? Creating slugs without the whole striptags and PageModel stuff still makes sense to me.

@aschempp
Copy link
Member

We cannot remove the contao.slug.generator service, this would break backwards compatibility.

Can we not decorate it?

@ausi
Copy link
Member

ausi commented Jan 23, 2018

We can. We could also replace the service with our own class that extends Ausi\SlugGenerator\SlugGenerator.

@backbone87
Copy link
Contributor

is there an interface for your SlugGenerator?

@ausi
Copy link
Member

ausi commented Jan 23, 2018

No.

@leofeyer leofeyer added the bug label Jan 26, 2018
@leofeyer leofeyer assigned leofeyer and ausi and unassigned leofeyer Jan 26, 2018
@leofeyer leofeyer added this to the 4.5.4 milestone Jan 26, 2018
@leofeyer leofeyer removed this from the 4.5.4 milestone Feb 14, 2018
@leofeyer leofeyer added feature and removed bug labels Feb 21, 2018
@leofeyer leofeyer added this to the 4.6.0 milestone Feb 21, 2018
@leofeyer leofeyer assigned leofeyer and unassigned ausi Feb 21, 2018
@leofeyer
Copy link
Member

leofeyer commented Feb 21, 2018

As discussed in the developer's meeting, this is what we want:

// tl_form_field.php
public function generateAlias($varValue, DataContainer $dc)
{
	$autoAlias = false;

	// Generate an alias if there is none
	if ($varValue == '')
	{
		$autoAlias = true;
		$slugOptions = array();

		// Read the slug options from the associated page
		if (($objPage = PageModel::findWithDetails($dc->activeRecord->jumpTo)) !== null)
		{
			$slugOptions = $objPage->getSlugOptions();
		}

		$varValue = System::getContainer()->get('contao.slug.generator')->generate(StringUtil::prepareSlug($dc->activeRecord->title), $slugOptions);
	}

New methods:

  • PageModel::getSlugOptions()
  • StringUtil::prepareSlug()

The prepareSlug() method does the following:

  1. strip insert tags
  2. restore basic entities
  3. decode entities

@leofeyer
Copy link
Member

Implemented in 9d7bdf4.

leofeyer added a commit to contao/calendar-bundle that referenced this issue Apr 11, 2018
leofeyer added a commit to contao/newsletter-bundle that referenced this issue Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants