Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Pass null/falsy values through Model.getIdentifier #3131

Merged

Conversation

dsevillamartin
Copy link
Member

Fixes #2876

Changes proposed in this pull request:

  • Return model in Model.getIdentifier if falsy

Reviewers should focus on:

  • Do we just want to handle null or all falsy values?
    • I think handling all is better as they're still visible in the request data, so should be easy to debug
    • I also think we should pass through the actual value instead of returning null, so that we don't modify the data (would add confusion)
  • Do we want to handle falsy/non-object model.data?
    • Personally I don't think so, as that would probably be an actual bug and not intentional. Having the error appear is probably the preferred situation.
  • Do we want an if statement instead to make the code clearer?
  • Do we want to handle this in getIdentifier method?

Screenshots
Before
image

After
image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

I think I would slightly prefer an explicit if-return for falsy model instances, but that's minor. Otherwise LGTM!

@dsevillamartin dsevillamartin merged commit 497dcce into master Nov 1, 2021
@dsevillamartin dsevillamartin deleted the ds/2876-pass-null-values-through-model-getidentifier branch November 1, 2021 15:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model.save() cannot save null hasOne relationship
3 participants