Skip to content

fix: morphTo when relation name is in camel case #2318

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

Closed
wants to merge 3 commits into from

Conversation

willtj
Copy link
Contributor

@willtj willtj commented Sep 24, 2021

Commit e19e10a in this PR updates existing tests to use a 'multi-word' relation for the morph relationship. This means that:

  • imageable is changed to has_image when passed as the $name parameter to morphOne() and morphMany()
    Note that snake case is required here - the parent Eloquent getMorphs() method constructs the expected column names by appending '_id' and '_type' to the name that's passed.
  • imageable is changed to hasImage when accessed as the relation name

That commit causes RelationsTest::testMorph() to fail, then commit c02e584 fixes the issue in line with how Eloquent's morphTo() method works, where $name variable defaults to the name of the calling function and is only snake-cased when passed to getMorphs() (reference)

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2021

Codecov Report

Merging #2318 (e26c877) into master (6aa6ad1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2318   +/-   ##
=========================================
  Coverage     88.12%   88.12%           
  Complexity      664      664           
=========================================
  Files            33       33           
  Lines          1566     1566           
=========================================
  Hits           1380     1380           
  Misses          186      186           
Impacted Files Coverage Δ
src/Eloquent/HybridRelations.php 93.58% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aa6ad1...e26c877. Read the comment docs.

Copy link
Contributor

@divine divine left a comment

Choose a reason for hiding this comment

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

Hello,

Honestly, this might break some applications which were depending on the "old" behavior even though it's incorrect/buggy.

We will probably keep this PR open until a new version with breaking changes will be ready.

Sorry, but no ETA currently.

Thanks!

@divine
Copy link
Contributor

divine commented Aug 26, 2023

This was merged in #2498.

Thanks!

@divine divine closed this Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants