Feature/1713 - Replace slugify to symfony string #1827

eerison · 2025-10-24T16:48:33Z

Replace slugify to symfony string

Closes #1713.
Related PR: #1718.

Changelog

### Added
- `FixPageUrlService` contains the logic from `$pageManager->fixUrl($page)`
- `FixPageUrlInterface` was added.
 
### Deprecated
- `SlugifyInterface` has been deprecated in favour of the`SluggerInterface` from `symfony/string`

src/Service/FixPageUrlService.php

src/Validator/UniqueUrlValidator.php

eerison · 2025-10-25T17:31:36Z

hey @VincentLanglet

do you know why doctrine.orm.entity_listener does not work 😄 ?

Edit: I used Entity/BasePage (it worked)

src/Listener/FixPageUrlLister.php

VincentLanglet

I feel like there is to many changes in a single PR.

If the purpose is to replace suglify by symfony string, why not JUST:

changing SlugifyInterface into SlugifyInterface|SluggerInterface
Updating the code to work with both
Deprecating SlugifyInterface

?

Currently you are

Introducing a new interface
Introducing a new class
Allowing SlugifyInterface|FixPageUrlInterface in PageManager
Adding a deprecation
Introducing an Adapter
Introducing a Factory
Introducing a Listener
And removing a fixUrl call in the validator

Be aware that sonata-project repository have almost no maintainer, so the smaller are the change, the better it is

eerison · 2025-10-26T09:15:44Z

I feel like there is to many changes in a single PR.

If the purpose is to replace suglify by symfony string, why not JUST:

changing SlugifyInterface into SlugifyInterface|SluggerInterface

Updating the code to work with both

Deprecating SlugifyInterface

?

Currently you are

Introducing a new interface

Introducing a new class

Allowing SlugifyInterface|FixPageUrlInterface in PageManager

Adding a deprecation

Introducing an Adapter

Introducing a Factory

Introducing a Listener

And removing a fixUrl call in the validator

Be aware that sonata-project repository have almost no maintainer, so the smaller are the change, the better it is

Yeah I know it is a bit big , I was trying to avoid it.

SlugifyInterface|SluggerInterface

It was my first thought, but I wanted to move this logical for a separated layer. But extracting the logical lead for a PR with too many things.

I was trying to avoid change the arguments, and later change again. But I will see how to make changes small. Maybe I back with the first solution accepting both Slug interface

eerison · 2025-10-26T18:26:28Z

@VincentLanglet could you take a look.

I just extracted the fixUrl logic from PageManager to FixPageUrlService, and yes I keeped 2 classes in BC layer.

eerison · 2025-10-26T18:31:08Z

src/Entity/PageManager.php

+                self::class,
+                FixPageUrlInterface::class,
+            ), \E_USER_DEPRECATED);
+            $fixPageUrl = FixPageUrlSlugifyFactory::create($fixPageUrl);


Maybe this factory could be removed, since it is used only here and it won't be reused in the code. it will be like this

$fixPageUrl = new FixPageUrlService(new SluggerSlugifyAdapter($slugify));

Since you're already planning to pass a Symfony slugger to this service, we could already have a small change of behavior here, so I don't think we have to force the use of sluggify here, you could

(new FixPageUrlService(new AsciiSlugger()))->fix()

Well it will make the things easier, I do not need to keep support for slugify.

But don't you think we should keep an option to use Slugify in case someone find an issue and want to keep the old way?

or if someone want to use the old version, they should implement an SlugifyAdapter in their side?

In next major we will remove sluggify anyway so if someone encounter an issue with the change it's better to fix it instead of keeping slugify support

VincentLanglet · 2025-10-26T19:30:40Z

src/Entity/PageManager.php

        string $class,
        ManagerRegistry $registry,
-        private SlugifyInterface $slugify,
+        private SlugifyInterface|FixPageUrlInterface $fixPageUrl,


why not changing this signature to

private SlugifyInterface|SluggerInterface

?

Either it make sens to introduce a FixPageUrl service and this should be done in a separate PR in which the name will be challenged, either it is not needed and we could simplify this pr

Currently,

I see no reason to introduce a FixPageUrlInterface because we don't plan having multiple implementation

PageManager::fixUrl is basically useless since you can call FixPageUrl::fix directly (Which is not a bad thing but then the fixUrl method could be deprecated and the service injection)

private SlugifyInterface|SluggerInterface

if you see in my first try, I added some if statement to check which interface I am getting, and It makes the code more complex.

edit: those managers are like repositories: https://github.com/sonata-project/sonata-doctrine-extensions/blob/2.x/src/Entity/BaseEntityManager.php. and this logical should be in other layer.

Either it make sens to introduce a FixPageUrl service and this should be done in a separate PR in which the name will be challenged, either it is not needed and we could simplify this pr

But don't you think this PR is simple enough to review? I basically extracted the logical and I am injecting it.

I see no reason to introduce a FixPageUrlInterface because we don't plan having multiple implementation

we do not plan to have multi implementation in our code. But it will help the devs to override the code if they need.

PageManager::fixUrl is basically useless since you can call FixPageUrl::fix directly (Which is not a bad thing but then the fixUrl method could be deprecated and the service injection)

yep that is true. but I am planning to deprecate this in other PR.

Related PR: sonata-project#1718 Issue sonata-project#1713

VincentLanglet · 2025-10-27T13:10:13Z

src/Service/FixPageUrlService.php

+use Sonata\PageBundle\Service\Contract\FixPageUrlInterface;
+use Symfony\Component\String\Slugger\SluggerInterface;
+
+final class FixPageUrlService implements FixPageUrlInterface


I think we should go with

final class PageFixer impelments PageFixerInterface {}

and a method PageFixerInterface::fixUrl

as it is a generic name, probably it will contain more methods with more fix. then you prefer have everything centralized in one place, even if it increases for 3 or 5 methods. and if we see it is necessary we refactor.

I do not have problem to change to PageFixer I just want to get your idea :). it will help for future PRs 😄

other question:

without suffix Service? if I keep inside of Service it will be different of other classes that already exist.

edit: it is mixed, there are services inside of src/Page/Service, src/Bock and src/Service

edit 2: I will keep with Service suffix as others services uses the same way, is it ok?

I personally hate the suffix service, didn't know some of the had.
On the opposite, other service like PageManager don't have such suffix.

We could create a folder Fixer ? with:

src/Fixer/PageFixerInterface src/Fixer/PageFixer

WDYT ?

in src/ level we should keep things like

Adapter

Bridge

Serializer

Http

Runtime

Service

Listener

etc I guess you know what I mean 😄 .. Fixer does not sound good IMO in src/ level.

I guess inside of Services fits good, But wihout Suffix, maybe we need to decide to keep or not Suffix.

well the problem in not using suffix, is it is in PSR for others things like MyCodeInterface.

eerison commented Oct 24, 2025

View reviewed changes

src/Service/FixPageUrlService.php Show resolved Hide resolved

eerison commented Oct 24, 2025

View reviewed changes

src/Service/FixPageUrlService.php Show resolved Hide resolved

eerison commented Oct 24, 2025

View reviewed changes

src/Validator/UniqueUrlValidator.php Outdated Show resolved Hide resolved

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch 2 times, most recently from fa5bd18 to 1b2b50f Compare October 25, 2025 09:27

eerison mentioned this pull request Oct 25, 2025

Remove fixUrl from UniqueUrlValidator #1828

Closed

6 tasks

eerison commented Oct 25, 2025

View reviewed changes

src/Listener/FixPageUrlLister.php Outdated Show resolved Hide resolved

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch from 005f0e5 to 47641d2 Compare October 25, 2025 18:07

VincentLanglet reviewed Oct 25, 2025

View reviewed changes

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch 9 times, most recently from 261d840 to 9d6aa9e Compare October 26, 2025 18:11

eerison marked this pull request as ready for review October 26, 2025 18:23

eerison requested a review from VincentLanglet October 26, 2025 18:27

eerison commented Oct 26, 2025

View reviewed changes

eerison changed the title ~~Feature/1713 - Replace slugify for symfony string~~ Feature/1713 - Replace slugify to symfony string Oct 26, 2025

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch 2 times, most recently from ab6ed87 to 181d376 Compare October 26, 2025 18:34

VincentLanglet reviewed Oct 26, 2025

View reviewed changes

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch 2 times, most recently from cbad503 to dab31e4 Compare October 27, 2025 11:52

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch from dab31e4 to 729d077 Compare October 27, 2025 11:54

Feature/1713 - Replace slugify for symfony string

c6205ac

Related PR: sonata-project#1718 Issue sonata-project#1713

eerison force-pushed the feature/1713-rework-replace-slugify-to-symfony-string branch from 729d077 to c6205ac Compare October 27, 2025 11:57

eerison requested a review from VincentLanglet October 27, 2025 12:00

VincentLanglet reviewed Oct 27, 2025

View reviewed changes

Uh oh!

Uh oh!

Feature/1713 - Replace slugify to symfony string #1827

Are you sure you want to change the base?

Feature/1713 - Replace slugify to symfony string #1827

Uh oh!

Conversation

eerison commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Replace slugify to symfony string

Changelog

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerison commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

eerison commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerison commented Oct 26, 2025

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!

VincentLanglet Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerison Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

eerison Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eerison commented Oct 24, 2025 •

edited

Loading

eerison commented Oct 25, 2025 •

edited

Loading

eerison commented Oct 26, 2025 •

edited

Loading

VincentLanglet Oct 26, 2025 •

edited

Loading

eerison Oct 27, 2025 •

edited

Loading

eerison Oct 27, 2025 •

edited

Loading