-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Replace the behat/transliterator with symfony/string for the Sluggable extension #2944
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
Conversation
$result = call_user_func_array( | ||
$this->sluggable->getUrlizer(), | ||
[$result, $separator, $object] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the urlizer here is the only way I could work out fixing this test failure:
1) Gedmo\Tests\Sluggable\Handlers\BothSlugHandlerTest::testSlugUpdates
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'web/developer/upd-gedi'
+'web/developer/upd gedi'
/tests/Gedmo/Sluggable/Handlers/BothSlugHandlerTest.php:64
|
||
$russian = $repo->findOneBy(['code' => 'ru']); | ||
static::assertSame('eto-testovyi-zagolovok-ru', $russian->getSlug()); | ||
static::assertSame('eto-testovyj-zagolovok-ru', $russian->getSlug()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just transliterated differently between packages. No idea which is more accurate, but given this is the only test case I actually did have to change, figured I'd point it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This transliteration complies with the ISO 9:1995 standard, so your change is spot on.
+1 thanks for the fix ! |
When can we expect this to be done? |
Up, thanks |
@phansys Could you please give some feedback on this, if you have some time. |
Update, please |
Is there anything we can do to help merge this PR ? |
The failing CI has nothing to do with this PR, I know I have one that fixes the main failure but at this point there's probably more stuff from newer ORM, DBAL, and MongoDB ODM releases that probably create new SA issues. |
If needed I will be able to help on solve the SA issues. But first I think we need to find an active maintainer. I see you're listed in the sponsor list of this repository, do you have merge rights @mbabker ? If not do you have any contact about someone which has ? I feel like this lib is lacking maintainer/activity... |
I'm pretty sure the only current SA issues are from the releases of ORM 3.4 and MongoDB ODM 2.11 (the ODM bits are covered in #2945, the ORM stuff might be covered in #2966 otherwise that shouldn't be anything more than reversing some inline
If I did, there'd be a lot more recently merged PRs and releases. I'm just the lucky one who's done a lot of reviewing and updating the library over the last year or so and someone else decided to add me to the sponsor list (which I'm absolutely grateful for, but also know I didn't ask for it). |
If you're ok with it, it could be interesting to contact existing maintainer to add you (?) |
Sorry for the delay in responding. I've been pretty busy at work these weeks. I'll try to take a look on this. |
I am closing and reopening this PR in order to trigger fresh pipelines. |
Fixes #2941