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

Revert return value change when column value is null #474

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

vencelkatai
Copy link
Contributor

Hi,

I've removed the modifications from getTranslation and modified the test to reflect that.
I've also tested the modified test in a prior version (6.8.0) to verify that the return value was an empty string before.

Fixes #473

@freekmurze freekmurze merged commit 5f07127 into spatie:main Dec 16, 2024
19 checks passed
@freekmurze
Copy link
Member

Thanks!

@vencelkatai vencelkatai deleted the fix/empty-translation-value branch December 16, 2024 15:52
@negoziator
Copy link

Thanks ✨

@alipadron
Copy link
Contributor

@vencelkatai I'd like to suggest a middle ground where we can both be satisfied. We could make this behavior configurable in the Translatable class. What about adding an option like $returnNullWhenColumnValueIsNull or something similar?

This would allow each developer to decide whether they want to get an empty string or null when the column has no translation.

Additionally, for consistency, we could consider making null the default value, as it's more explicit about the absence of data.

From my perspective, it's inconsistent to return an empty string when the column value is null, as null can be used to indicate that there's no translation, allowing for specific actions based on this condition.

I understand that returning an empty string for null values might work for your use case, but it doesn't align with my mental model.

I spent quite some time trying to figure out why, in models where the column has a default null value (since it's not mandatory), I was getting an empty string as the translation.

I'd be curious to hear @freekmurze thoughts on this regarding the package design.

I am writting this because i believe it's unfair to revert changes without a justifiable cause

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.

Latest changes to attribute mutator breaks getAttribute() behavior
4 participants