craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.29k stars 638 forks source link

limitAutoSlugsToAscii do not work #10012

Closed jan10 closed 3 years ago

jan10 commented 3 years ago

Description

I have enabled the following setting in general.php:

 'limitAutoSlugsToAscii ' => true,

When I create a new page with "ä", the slug is not converted to "ae":

image

Steps to reproduce

  1. Add 'limitAutoSlugsToAscii ' => true, to config.php
  2. Add a new entry with ä

Additional info

Contact Form 2.2.7 Image Resizer 2.1.0 Imager X v3.5.3 Minify 1.2.10 Redactor 2.8.8 SEO 3.7.4

brandonkelly commented 3 years ago

Just tested locally and it’s working for me. Are you typing the ä directly into the Slug field, or the Title? That setting will only affect automatically-generated slugs, based on the Title field.

jan10 commented 3 years ago

In the Title field. I'll test it locally with a new installation and get back to you.

jan10 commented 3 years ago

With a fresh new install over nitro and this config i can also reproduce the error.

<?php
/**
 * General Configuration
 *
 * All of your system's general configuration settings go in here. You can see a
 * list of the available settings in vendor/craftcms/cms/src/config/GeneralConfig.php.
 *
 * @see \craft\config\GeneralConfig
 */

use craft\helpers\App;

$isDev = App::env('ENVIRONMENT') === 'dev';
$isProd = App::env('ENVIRONMENT') === 'production';

return [
    // Default Week Start Day (0 = Sunday, 1 = Monday...)
    'defaultWeekStartDay' => 1,

    // Whether generated URLs should omit "index.php"
    'omitScriptNameInUrls' => true,

    // The URI segment that tells Craft to load the control panel
    'cpTrigger' => App::env('CP_TRIGGER') ?: 'admin',

    // The secure key Craft will use for hashing and encrypting data
    'securityKey' => App::env('SECURITY_KEY'),

    // Whether Dev Mode should be enabled (see https://craftcms.com/guides/what-dev-mode-does)
    'devMode' => $isDev,

    // Whether administrative changes should be allowed
    'allowAdminChanges' => $isDev,

    // Whether crawlers should be allowed to index pages and following links
    'disallowRobots' => !$isProd,

    'limitAutoSlugsToAscii ' => true
];

image

jan10 commented 3 years ago

Sorry, my mistake. I had a space between limitAutoSlugsToAscii and '.

brandonkelly commented 3 years ago

Ah yep, that’ll do it. Glad you got it sorted!

jan10 commented 3 years ago

Maybe there will be a check for incorrect/unkown configuration in one of the upcoming Craft CMS releases :)

brandonkelly commented 3 years ago

@Jan10 Yeah… craft\config\GeneralConfig (the class that stores all general config settings) is currently capable of storing custom settings, because that was a pretty common thing to do in the Craft 2 days. So that’s why it’s not complaining here.

Environment variables or custom module settings are better alternatives nowadays though, so I have removed support for custom config settings for Craft 4.

(I considered a partial solution for now: to start throwing an exception when a custom config setting is being set, if its name doesn’t even match an allowed property name, such as in this case with the extra space, however I’m worried it would cause unexpected errors for sites out there which may be making the same mistake unknowingly.)

aelvan commented 3 years ago

Ouch, that is one extremely painful breaking change. What will happen if you have custom settings after upgrading to 4.0? Will Craft throw an exception on page load? Or will it just not return a value when trying to access it?

brandonkelly commented 3 years ago

@aelvan It will throw an UnknownPropertyException, like any other case where you try to set a nonexistent property on an object that extends yii\base\BaseObject.

This is far from the first time someone has gotten bit by GeneralConfig’s tolerance for invalid setting names – usually due to a casing mistake or a misspelling. Considering there are more appropriate ways to set custom values now, I think it’s doing more harm than good to keep it around.

mmikkel commented 3 years ago

Environment variables or custom module settings are better alternatives nowadays though, so I have removed support for custom config settings for Craft 4.

Compared to what we can currently achieve with a single line of PHP, custom module settings require a ton of boilerplate – especially if the values need to be exposed to Twig. Don't know if this will be a feasible alternative for most sites.

Using environment variables for everything is also not a particularly good option. If a value is neither sensitive (like an API key) nor truly environment specific (like a local filepath), it probably belongs in version control and not in the .env file.

I'm not suggesting this change should be walked back, but I'm wondering if it'd be possible to introduce an additional way of adding custom config values to a project, that more closely matches the current ability. Perhaps a customSettings array could be added to the general config or something like that?

brandonkelly commented 3 years ago

Not a bad idea @mmikkel. How would you feel if it were in its own file though, e.g. config/custom.php? Then from your templates you’d just be changing general to custom whenever accessing craft.app.config.general.myCustomSetting.

mmikkel commented 3 years ago

@brandonkelly I think a config/custom.php file would be the ideal solution, even better than having custom config settings support in general.php in the first place 👍

brandonkelly commented 3 years ago

Done: #10100

mmikkel commented 3 years ago

This is great. Thanks @brandonkelly!