Skip to content

[String] Slugger: Adding setSymbolsMap() #44199

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

Closed
wants to merge 1 commit into from

Conversation

ThomasLandauer
Copy link
Contributor

Q A
Branch? 5.4 for features
Bug fix? no
New feature? yes
Deprecations? no
Tickets None
License MIT
Doc PR None yet

Am I right that there is no way to configure the $ymbolsMap when using the Symfony framework integration (i.e. dependency injection)?
What about adding a public method for this?
The feature to set it in the constructor was added in #36456

Waiting for feedback before continuing.

Am I right that there is no way to configure the `$ymbolsMap` when using the Symfony framework integration (i.e. dependency injection)?
What about adding a public method for this?
The feature to set it in the constructor was added in symfony#36456
@carsonbot carsonbot added this to the 5.4 milestone Nov 22, 2021
@ThomasLandauer ThomasLandauer changed the title Adding setSymbolsMap() [String] Slugger: Adding setSymbolsMap() Nov 22, 2021
@ThomasLandauer
Copy link
Contributor Author

Besides, it would be nice to be able to set global options for the slugger (i.e. at https://symfony.com/doc/current/reference/configuration/framework.html) - just like cocur/slugify allows: https://github.com/cocur/slugify#symfony-configuration

@xabbuh
Copy link
Member

xabbuh commented Nov 22, 2021

Besides, it would be nice to be able to set global options for the slugger (i.e. at https://symfony.com/doc/current/reference/configuration/framework.html)

Given the reason why you wanted to introduce this method I think it would make more sense to introduce that possibility than adding the setter method.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

I believe it was an intentional design decision to make the symbols map immutable. The map can be set via the constructor and you could easily wire your own configuration of the slugger if you need a different map.

Making the map configurable in FrameworkBundle could also be feasible, but I'm afraid that it's a bit too late to squeeze that into the already frozen 5.4 release.

@carsonbot carsonbot changed the title [String] Slugger: Adding setSymbolsMap() Slugger: Adding setSymbolsMap() Nov 22, 2021
@carsonbot carsonbot changed the title Slugger: Adding setSymbolsMap() [String] Slugger: Adding setSymbolsMap() Nov 22, 2021
@nicolas-grekas
Copy link
Member

Yep, this is intentional: having the service immutable is desired.
I guess it's possible to wire a new value in a compiler pass, or by replacing the service definition, isn't it?
Dunno if this is common enough to be added to configuration though.
To be discussed for 6.1.

@ThomasLandauer
Copy link
Contributor Author

Setter vs. configuration: Well, both would be nice ;-) If you're using the slugger in 100 services, you need the configuration; if you have one exception, you need the setter ;-)

Version: I didn't really mean to get this into 5.4 ;-)

This is the default $symbolsMap:

'en' => ['@' => 'at', '&' => 'and'],

... and this looks like a "template" to me, showcasing how to set it up. So at least an easy way to get '&' => 'and' in more languages would be nice :-)

This is the first Symfony feature I'm seeing where the framework integration is harder to use than the standalone (cause you can't just pass it to the constructor). Shouldn't this be the other way round?
But maybe the following is the answer and it's just not documented:

"wire your own configuration" or "replacing the service definition": How does this work?

@ThomasLandauer ThomasLandauer marked this pull request as draft November 23, 2021 00:23
@stof
Copy link
Member

stof commented Nov 23, 2021

if you have one exception, you need the setter ;-)

No. You need a separate instance. If you use the setter for this exception, you also affect the usages done in other services after you called that setter.
That's the main reason why we avoid setters on services. It makes things a lot harder to reason about.

@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 29, 2021
@fancyweb
Copy link
Contributor

Should we close this one?

@ThomasLandauer ThomasLandauer deleted the patch-4 branch December 31, 2021 15:08
@ThomasLandauer
Copy link
Contributor Author

Indeed, if I pass ->args() in my services.php, it works:

$services->set('slugger', AsciiSlugger::class)->args(['de', ['de' => ['&' => 'und']]]);

However, if I pass ->arg() it does not work:

$services->set('slugger', AsciiSlugger::class)->arg('$symbolsMap', ['de' => ['&' => 'und']]);

Does anybody have an idea, why?
The argument name $symbolsMap is correct, see https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/String/Slugger/AsciiSlugger.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants