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

[WIP] Add the contao.slugify service #569

Closed
wants to merge 4 commits into from
Closed

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer commented Aug 12, 2016

TODO

  • Move the "folderUrl" setting into the application configuration
  • Set the slugify regex depending on the "folderUrl" setting
  • Replace the Utf8::toAscii() calls where applicable

@contao/developers /cc

@leofeyer leofeyer added this to the 4.3.0 milestone Aug 12, 2016
@leofeyer leofeyer self-assigned this Aug 12, 2016
@leofeyer
Copy link
Member Author

However, why are we slugifying at all? All browsers can handle UTF-8 URLs meanwhile, so we might as well just leave news/neue-größen-eingetroffen.html as is.

@leofeyer
Copy link
Member Author

leofeyer commented Aug 12, 2016

The PR requires the following code to be added in app/config/config.yml:

# Slugify
cocur_slugify:
    regexp: "#([^A-Za-z0-9/.]|-)+#"

This will add / and . to the list of allowed characters, because our aliases can be index.de or folder/urls/supported.

$strName = preg_replace('/[^A-Za-z0-9._-]/', '', $strName);
$strName = basename($strName);
$strName = \System::getContainer()->get('contao.slugify')->slugify($objTheme->name);
$strName = str_replace(array('-/', '/-', '.-', '.'), array('', '', '-', ''), $strName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nasty. We need the service to be aware of Config::get('folderUrl'), which means we first have to move the setting into the application configuration.

@leofeyer
Copy link
Member Author

I have updated the PR so it initializes the Slugify regex depending on the contao.folder_urls setting. However, it still feels wrong.

The initial problem is the Slugify service not providing a setOption() method IMO.

@aschempp
Copy link
Member

The ruleset stuff feels wrong. We're always activating rulesets, but they are never removed?

@leofeyer
Copy link
Member Author

leofeyer commented Aug 15, 2016

The ruleset stuff is wrong. See cocur/slugify#133 😄

@aschempp
Copy link
Member

aschempp commented Dec 6, 2016

I think this PR is outdated or am I wrong?

@leofeyer
Copy link
Member Author

leofeyer commented Dec 6, 2016

You are wrong. Our changes have been merged but there has not been a release since then. As soon as a new version is released, I will complete the PR.

@Toflar
Copy link
Member

Toflar commented May 29, 2017

To be honest, I think it depends. It's something that should be a setting of a root page. If you think in German, somebody might want to have URI's like foobar.de/über-uns.html and an other one would like to have foobar.de/ueber-uns.html. It's a matter of taste. Also I think prepend_locale as well as folderUrls are both settings that should be a root page setting, not a system setting. There are other tickets where people would like to have the main language to be accessible via www.domain.de and all translations via www.domain.de/{locale}. If it was configurable in the root page settings, I guess this would be possible too then. So I think we should mark that as a feature for 4.5.

@leofeyer
Copy link
Member Author

If folderUrls was a root page setting, how would the resolver know whether to look for folder URLs or not?

@aschempp
Copy link
Member

I don't think that's currently possible, I guess the Symfony CMF-Router can solve this, but as we know that's not a simple task.

@leofeyer
Copy link
Member Author

I'm closing this in favor of #1016.

@leofeyer leofeyer closed this Aug 15, 2017
@leofeyer leofeyer deleted the feature/slugify branch August 15, 2017 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants