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

Allow to return null on native cast #152

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Allow to return null on native cast #152

merged 2 commits into from
Jul 24, 2020

Conversation

rodrigopedra
Copy link
Contributor

@rodrigopedra rodrigopedra commented Jul 15, 2020

  • Added or updated tests
  • Added or updated the README.md
  • Detailed changes in the CHANGELOG.md unreleased section

Related Issue/Intent

When using the native casting introduced in v2.0 I noticed that returning null from the parseDatabase method would result in error as it would still try to create an Enum instance from the returned value.

My use case is a modernization of a legacy codebase (25 years old) where some blank data is stored as empty strings instead of NULLs in the database.

So when running code like this:

public static function parseDatabase($value)
{
    if (\blank($value)) {
        return null;
    }

     return (int) $value;
}

Application would throw an exception on blank values (null or empty string).

For this particular project I ended adding a NA enum value and defaulting to that for blank values. This is not an ideal solution as the code that relies on the Enum value, and code to persist values back needs to take account of that.

Changes

This PR changes the Casts/EnumCast class to check for null values returned by userland code when using the parseDatabase method.

A new test was added. Test would fail without the changes introduced by this PR.

Breaking changes

No breaking changes.

Copy link
Contributor

@atymic atymic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. cc @BenSampo

@rodrigopedra
Copy link
Contributor Author

rodrigopedra commented Jul 15, 2020

Hi @atymic thanks for the quick assessment.

The library code on EnumCast checks specifically for nulls ($value === null), so if userland code returns, for example, 0 it would still be cast to an enum instance.

But the test class I updated (UserTypeCustomCast) allows for a falsy value for the Administrator enum value. Therefore the serializeDatabase in that example would fail for that value.

I will update both the test enum class and the test case.

UPDATE: no changes needed.

@BenSampo BenSampo merged commit c1e4f59 into BenSampo:master Jul 24, 2020
@BenSampo
Copy link
Owner

Cheers @rodrigopedra , I'll tag that now

@BenSampo
Copy link
Owner

@rodrigopedra I've tweaked the readme slightly, can you check that it still makes sense:

https://github.com/BenSampo/laravel-enum#casting-underlying-native-types

@rodrigopedra
Copy link
Contributor Author

Thanks @BenSampo .

I saw the readme and your versions is much clearer, thanks again.

atymic pushed a commit to atymic/laravel-enum that referenced this pull request Oct 11, 2020
* allow to return null on custom cast

* changed readme and changelog
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.

3 participants