Skip to content

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