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

[8.x] Fix translation bug and add test #39298

Merged
merged 2 commits into from
Oct 21, 2021
Merged

[8.x] Fix translation bug and add test #39298

merged 2 commits into from
Oct 21, 2021

Conversation

taylorotwell
Copy link
Member

Fixes #39258

Uses strtr function instead of str_replace. strtr internally sorts keys by length so sort replacements can be removed as well.

@taylorotwell taylorotwell merged commit 024067f into 8.x Oct 21, 2021
@taylorotwell taylorotwell deleted the fix-transalation-bug branch October 21, 2021 16:07
@GrahamCampbell GrahamCampbell changed the title fix translation bug and add test [8.x] Fix translation bug and add test Oct 21, 2021
@cgaube
Copy link

cgaube commented Oct 25, 2021

@taylorotwell
Hello - I was wondering about those kind of changes.
You removed a function that is "protected", that's probably a breaking change right ? - Anybody extending the translator and using that function that was removed now will trigger a Fatal error.

So yeah i was wondering about the process - the function should be probably marked as deprecated and left empty and removed in the next version (9)

Am I totally off-base ?

@driesvints
Copy link
Member

@cgaube are you affected by this?

@cgaube
Copy link

cgaube commented Oct 25, 2021

@driesvints yes - I am extending the translator - and was using that function -
I was able to find this issue super fast thanks to my test coverage and it was an easy fix but it is still concerning to update laravel minor version and get some fatal errors :)

@carlobeltrame
Copy link

Same here, we are extending translator as well, and the patch upgrade of Laravel broke our tests. Also an easy fix here, but technically this is a breaking change.

@driesvints
Copy link
Member

driesvints commented Jan 24, 2022

I'm afraid we won't be reverting this. We were fixing a bug here and as always, when you're overwriting core functionality you have to be aware that in some very edge cases it can break your specific use case. I'm sorry you both got caught with this but we believe this bug warranted fixing.

@cgaube
Copy link

cgaube commented Jan 26, 2022

Oh yeah I was never suggesting that we revert those changes.

It was more of a question about how Laravel handle breaking changes - Usually when introducing one you update to the next major version, and you document any breaking change in a CHANGELOG or something.

I was just curious about the process for this project - because if laravel is ok with introducing breaking change on minor updates it will require extra care for us when updating.

Fortunately In my project every laravel services that we had to extend have a feature or unit test associated to catch those kind of issues.

Thank you

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.

Translator parses replacements in $replace data if key is sorted after the current key.
4 participants