Skip to content
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

[String] Transliterate & to and #35689

Merged
merged 1 commit into from Feb 13, 2020
Merged

[String] Transliterate & to and #35689

merged 1 commit into from Feb 13, 2020

Conversation

@Warxcell
Copy link
Contributor

Warxcell commented Feb 12, 2020

Q A
Branch? 5.0
Bug fix? kind of
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR
@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Feb 12, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 12, 2020

Can you please add a test case?

@Warxcell

This comment has been minimized.

Copy link
Contributor Author

Warxcell commented Feb 13, 2020

@nicolas-grekas I think it's also a good idea to be able to add custom behaviour in that slugger (for such replacements). I was thinking about callback. What do you think?

@nicolas-grekas nicolas-grekas changed the title Transliterate & to and [String] Transliterate & to and Feb 13, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 13, 2020

be able to add custom behaviour in that slugger

I don't think it's needed: decoration alreadt allows implementing custom behaviors to me.

@fabpot
fabpot approved these changes Feb 13, 2020
@fabpot fabpot changed the base branch from 5.0 to master Feb 13, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 13, 2020

Thank you @Warxcell.

fabpot added a commit that referenced this pull request Feb 13, 2020
This PR was submitted for the 5.0 branch but it was squashed and merged into the 5.1-dev branch instead.

Discussion
----------

[String] Transliterate & to and

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | kind of
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

Commits
-------

76ff984 [String] Transliterate & to and
@fabpot fabpot merged commit 76ff984 into symfony:master Feb 13, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@Warxcell

This comment has been minimized.

Copy link
Contributor Author

Warxcell commented Feb 13, 2020

@nicolas-grekas kinda. You have to make your own instance of UnicodeString before AsciiSlugger,
then pass result from ->toString() to AsciiSlugger. Or just use regular str_* or mb_* functions.

    public function slug(string $string, string $separator = '-', string $locale = null): AbstractUnicodeString
    {

        $string = new UnicodeString($string);
        $string->replace('XX', 'YY');
        //  do other stuffs

        return $this->asciiSlugger->slug($string->toString(), $separator, $locale);
    }
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 13, 2020

yes
or you replace on the return point (return parent::slug(...)->replace(...);)
or you reimplement completely
etc

@Warxcell

This comment has been minimized.

Copy link
Contributor Author

Warxcell commented Feb 13, 2020

@nicolas-grekas no, not exactly. on return point everything is now replaced by $separator - it's too late to do anything else on this string.
The other possibility is method to accept either string, either AbstractUnicodeString.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 13, 2020

Oh, indeed. The string barrier is not an issue to me, it's fine creating the object again inside.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.