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

Fix for Issue #455 (Adding support for UTF-8 slug) #456

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Fix for Issue #455 (Adding support for UTF-8 slug) #456

merged 4 commits into from
Feb 27, 2018

Conversation

darron1217
Copy link
Contributor

Fix for Issue #455 (Adding support for UTF-8 slug)

@nWidart
Copy link
Member

nWidart commented Jan 15, 2018

Looks like the tests fail b/c the file isn't required in the tests.

I'm also thinking maybe this is something that should be sent to laravel instead?

@darron1217
Copy link
Contributor Author

@nWidart
Laravel rejected similar PR... :(
They recommend implementing utf8 slug method by ourselves

laravel/framework#6867

@nWidart
Copy link
Member

nWidart commented Feb 26, 2018

Hello,

Could you add a test for this method?

@nWidart
Copy link
Member

nWidart commented Feb 27, 2018

Thanks, maybe it's also worth pulling in all the tests str_slug() has on its own?

@darron1217
Copy link
Contributor Author

@nWidart
Copy link
Member

nWidart commented Feb 27, 2018

That looks like it indeed. Since we're replicating that method just to make sure the 'base' cases work, and keep working in the future 😄

Maybe have one test with all the new ones, plus your new one added.

@darron1217
Copy link
Contributor Author

@nWidart I've done it :)

@nWidart
Copy link
Member

nWidart commented Feb 27, 2018

Thanks! 👍

@nWidart nWidart merged commit d72663e into AsgardCms:3.0 Feb 27, 2018
@darron1217 darron1217 deleted the issue#455 branch June 10, 2018 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants