Set default options to routes created in the route-providers #5561

niklasnatter · 2020-10-21T12:12:24Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Fixed tickets	fixes sulu/SuluArticleBundle#521
License	MIT

What's in this PR?

This PR adjusts the following route providers to include the symfony route default options:

RouteProvider of the RouteBundle
ContentRouteProvider of the WebsiteBundle
CustomUrlRouteProvider of the CustomUrlRouteBundle

The route default options were introduced in Symfony 4.2 to handle the framework.router.utf8 configuration: symfony/symfony#27774. This options allows to set the utf8 option to all router per default. This is needed when using UTF8 symbols inside of the path or the requirement of a route. Unfortunately, the symfony-cmf/routing package cannot handle this for us (see discussion below).

The utf8 option of the Route class was introduced in Symfony 3.2:

Why?

Because routes cannot use UTF-8 characters without setting the utf8 option (See sulu/SuluArticleBundle#521). For example, the path accééeeents leads to the following error:

Cannot use UTF-8 route patterns without setting the "utf8" option for route "/en/accééeeents3".

This is caused by the following lines:
https://github.com/symfony/symfony/blob/35dad22f9e01b06b538837b8c729d67f26c6f6ed/src/Symfony/Component/Routing/RouteCompiler.php#L113-L118

niklasnatter · 2020-10-21T12:18:40Z

I guess we should only add the utf8 option if framework.router.utf8 is configured to true. Do you have an idea how to do that? 🤔

alexander-schranz · 2020-10-27T10:07:33Z

Maybe this is something which should the Symfony CMF Routing Bundle should take care of if the utf8: true is set all the routes should also get that option. /cc @dbu what do you think?

dbu · 2020-10-27T13:08:46Z

@alexander-schranz that sounds reasonable. i guess at the time we built this (symfony 2) this option probably did not exist. do you want to do a pull request for it? we might need a compiler pass to "copy" the configuration over.

niklasnatter · 2020-10-29T15:49:37Z

@dbu Thanks for your quick response. I just had a look the code of the symfony-cmf/routing package, but I am not sure where I could set the utf8: true option to the route.

My idea was to implement this similar to the symfony implementation: symfony/symfony#27774. In that implementation, the DelegatingLoader loops over the loaded route collection and sets the default options (which include the utf8: true option): https://github.com/symfony/symfony/pull/27774/files#diff-c6bf228eca2bee9991fbaa09eefbbf51bb3396387f0724c46f19f4cc80285660R78-R80

What would be the correct service to do this in the symfony-cmf/routing or symfony-cmf/routing-bundle package? I already noticed that the NestedMatcher gathers the route collection from the provider: https://github.com/symfony-cmf/Routing/blob/3c97e7b7709b313cecfb76d691ad4cc22acbf3f5/src/NestedMatcher/NestedMatcher.php#L140

But as far as I see, this class is not registered as service in the symfony-cmf/routing-bundle but inside of the project. This means that I cannot pass any default options to this class, right?

Thanks for your time!

dbu · 2020-11-02T07:40:14Z

the idea of the cmf routing is that routes are stored in a database, as opposed to being statically configured.

is the utf8 option a thing on the route object, or more general? and what exactly is the effect of the flag?
if it is on the route object, storing it in the object would be possible but likely be weird as you probably want to have that globally. it might affect the route loading mechanism candidates and validity check... depending on the storage system, we might also need to encode the paths and decode them again, and then encode in the candidates strategy - i think at least PHPCR does not fully handle utf-8 in node names.

i hope this helps you. i am afraid that this could be quite a chain of changes in the end... we do not necessarily need it for all storage solutions provided by the cmf bundle, they already do not have exactly the same features.

niklasnatter · 2020-11-02T13:23:09Z

@dbu Thanks for your answer!

Yes, the utf8: true is just an option that is passed to the/saved on the Route. For example, it can be set when constructing a route manually (new Route('path', [], [], ['utf8' => true])) or by using the following annotation:

/**
 * @Route(
 *     "/category/{name}",
 *     "options" = { "utf8": true }
 * )
 */

The effect of the flag is that the PCRE_UTF8 modifier is added to the regex of the route. This allows to use unicode characters in the path and in the requirements of the route:
https://github.com/symfony/symfony/blob/35dad22f9e01b06b538837b8c729d67f26c6f6ed/src/Symfony/Component/Routing/RouteCompiler.php#L234-L241

With this in mind, here comes the problem: In 4.2, Symfony added a framework.router.utf8 configuration that sets ['utf8' => true] to all loaded routes per default. My goal would be to somehow respect this option in the symfony-cmf/routing-bundle. Meaning that I would like to set ['utf8' => true] to all routes returned by the registered providers.

At the moment, setting this default value to the routes when the framework.router.utf8 configuration is set would be enough for us. I think we dont need to change anything about storing/loading the routes 🙂

Do you see a place for handling this in the symfony-cmf/routing-bundle? Or should we just adjust the RouteProvider implementation inside of Sulu to respect the framework.router.utf8 configuration?

dbu · 2020-11-02T14:36:17Z

i see. then i think you should do it in the step that loads the route candidates from storage. (you will need to double-check if the candidates strategy works with utf8 routes though, as the point in the cmf route loading it to not always load potentially thousands or tens of thousands of routes)

niklasnatter · 2020-11-09T10:22:29Z

@sulu/core-team I adjusted this PR to to use compiler passes for copying the route default options to our route providers. I also updated the title and description to explain why this is necessary 🙂

alexander-schranz

I think to be sure that the ContentProvider is getting the correct defaultOptions we should have some kind of functional test case to test if the route have utf8=>true set. A mocked test I think is in this cases not enough as it would not test if the compilerpass works correctly when symfony does change the routing.loader service definition.

alexander-schranz · 2020-11-09T10:44:53Z

src/Sulu/Bundle/WebsiteBundle/DependencyInjection/Compiler/RouteDefaultOptionsCompilerPass.php

+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+class RouteDefaultOptionsCompilerPass implements CompilerPassInterface


Maybe instead of having multiple compilerpasses we should have one compilerpass in the route or core bundle which sets a parameter which could be just used by the other service definitions.

Yes i thought about that too, but where should I place it? 🙂

alexander-schranz · 2020-11-09T10:45:52Z

src/Sulu/Bundle/WebsiteBundle/DependencyInjection/Compiler/RouteDefaultOptionsCompilerPass.php

+        // https://github.com/symfony/symfony/pull/31900
+        $container->getDefinition('sulu_website.provider.content')->replaceArgument(
+            7,
+            $container->getDefinition('routing.loader')->getArgument(1)


Is it not possible to use getArgument('$defaultOptions') or something to be sure that we load no other argument instead?

Unfortunately not. The argument "$defaultOptions" doesn't exist.

Okay a functional test that the defaultOptions is correctly set is then for me more important, else in a symfony upgrade this is maybe forgotten to be updated.

added a functional test case 🙂

niklasnatter mentioned this pull request Oct 21, 2020

UTF8 routing not taken into account when saved as draft, then published. sulu/SuluArticleBundle#521

Closed

niklasnatter force-pushed the enhancement/route-utf8 branch from dd0cfad to 774dbfe Compare November 9, 2020 10:06

niklasnatter changed the base branch from release/2.1 to master November 9, 2020 10:11

niklasnatter force-pushed the enhancement/route-utf8 branch from 774dbfe to e8dbf2e Compare November 9, 2020 10:11

niklasnatter changed the title ~~Set utf8 option for routes created in the RouteProvider~~ Set default options to routes created in the route-providers Nov 9, 2020

alexander-schranz requested changes Nov 9, 2020

View reviewed changes

niklasnatter assigned alexander-schranz Nov 10, 2020

alexander-schranz approved these changes Nov 10, 2020

View reviewed changes

niklasnatter added 5 commits November 10, 2020 11:24

Set utf8 option for routes created in the RouteProvider

b5ceb2b

Add RouteDefaultOptionsCompilerPass to copy symfony default options

15dbaed

Add functional test case

463110b

Fix symfony 4.4 compatibility

49d53e4

Move compiler class file into components

725939e

niklasnatter force-pushed the enhancement/route-utf8 branch from 0077d7c to cb2ab85 Compare November 10, 2020 10:27

niklasnatter changed the base branch from master to release/2.1 November 10, 2020 10:27

Fix compatibility with symfony 4.4

aee6ddb

niklasnatter force-pushed the enhancement/route-utf8 branch from cb2ab85 to aee6ddb Compare November 10, 2020 10:27

Fix argument index in SuluWebsiteBundle

122dea4

alexander-schranz approved these changes Nov 10, 2020

View reviewed changes

alexander-schranz merged commit ab09901 into sulu:release/2.1 Nov 10, 2020

alexander-schranz mentioned this pull request Apr 23, 2025

Readd handling for route defaults options utf8 true #7912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set default options to routes created in the route-providers #5561

Set default options to routes created in the route-providers #5561

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Set default options to routes created in the route-providers #5561

Set default options to routes created in the route-providers #5561

Uh oh!

Conversation

Uh oh!

What's in this PR?

Why?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!