Fix compatibility with Laravel 10.30 — lowercase the `$passthru` array values #2661

Treggats · 2023-11-01T23:55:34Z

As explained in the issue, Laravel updated the way Builder methods are called. The list of methods in the $passthru property now should be lowercase.

Currently they are not, and this causes the methods to not be called, but passed on and thus returning the Builder instance.

This PR makes them all lowercase, which will fix this issue. I have tested this in our application and it works.

GromNaN

Hello @Treggats. Thank you for identifying and fixing this bug.
Could you add a test for each method to ensure we don't have regression?

src/Eloquent/Builder.php

GromNaN · 2023-11-02T08:31:23Z

src/Eloquent/Builder.php

-        'insertGetId',
-        'insertOrIgnore',
-        'insertUsing',
+        'insertgetid',


If #2659 is merged, this method will never be called. Would it be better to remove it? Possibly by leaving a comment.
Or maybe discard #2659 and keep this PR as a fix.

What you said in the comment in #2659 makes sense, the latter part.
Personally I like to be specific/remove magic, but that's the way Laravel works and we need to align with that.

In short, close #2659 in favour of this one.

It seems to be fixed by #2661. Both solutions will work, yours removes a layer of magics but it is more fragile if the implementation changes in Laravel.

Treggats · 2023-11-02T08:35:50Z

Hello @Treggats. Thank you for identifying and fixing this bug. Could you add a test for each method to ensure we don't have regression?

@GromNaN thanks for the review, I will add tests and address the review comments.

Treggats · 2023-11-02T11:14:05Z

a little sidenote, locally I made the following change. I won't commit it as its a personal preference though it doesn't seem to impact the current codebase.

diff --git a/phpcs.xml.dist b/phpcs.xml.dist
index 36cc870..23bc44a 100644
--- a/phpcs.xml.dist
+++ b/phpcs.xml.dist
@@ -34,5 +34,7 @@
         <exclude name="SlevomatCodingStandard.TypeHints.ParameterTypeHint"/>
         <exclude name="SlevomatCodingStandard.TypeHints.PropertyTypeHint"/>
         <exclude name="SlevomatCodingStandard.TypeHints.ReturnTypeHint"/>
+
+        <exclude name="Generic.Formatting.MultipleStatementAlignment" />
     </rule>
 </ruleset>

GromNaN · 2023-11-02T12:50:23Z

You can commit this change. It annoys me too.
It's excluded from mongodb-php-library.

Treggats · 2023-11-02T12:58:32Z

You can commit this change. It annoys me too. It's excluded from mongodb-php-library.

I will follow suit then.

hans-thomas · 2023-11-02T13:19:10Z

@GromNaN the #2656 issue is related to this PR too. After merging, don't forget to close it.

Treggats · 2023-11-02T13:20:32Z

@GromNaN the #2656 issue is related to this PR too. After merging, don't forget to close it.

updated the description so that it closes after merging of this PR

Treggats · 2023-11-02T13:31:37Z

src/Eloquent/Builder.php

        'max',
        'min',
        'pluck',
        'pull',
        'push',
        'raw',
        'sum',
-        'toSql',
+        'tomql',


replaced as it is not supported

Treggats · 2023-11-02T13:32:10Z

tests/Eloquent/CallBuilderTest.php

+        self::assertNotInstanceOf(expected: $className, actual: $builder->{$method}(...$parameters));
+    }
+
+    public static function provideFunctionNames(): Generator


provide supported methods, which should not return a Builder instance.

Treggats · 2023-11-02T13:33:53Z

tests/Eloquent/CallBuilderTest.php

+        $builder = User::query()->newQuery();
+        assert($builder instanceof Builder);
+
+        $this->expectException($exceptionClass);


verify that certain methods are not supported, checking both the exception type and its message

Treggats · 2023-11-02T14:19:27Z

Currently the MassPrunableTest::testPruneWithQuery test is failing locally, I'm trying to figure out why that is.

The $this->assertEquals(1, User::count()); assertion fails, two models were not pruned. Thus the count() query returns 3

GromNaN · 2023-11-02T14:21:40Z

While testing, I just realized that it breaks compatibility with illuminate/database < 10.30.0.

One strategy for maintaining compatibility would be to modify this list in __construct, adding only the specific methods, in lowercase and non-lowercase versions (leaving a comment that non-lowercase versions are kept for compatibility with Laravel < 10.30).

GromNaN · 2023-11-02T14:52:47Z

tests/Eloquent/CallBuilderTest.php

+        ];
+
+        yield 'dd' => [
+            'dd',


We can create a separate issue to support this, once this PR get merged.

tests/Eloquent/CallBuilderTest.php

codecov-commenter · 2023-11-02T16:18:22Z

Codecov Report

Merging #2661 (a14a822) into 4.0 (25bd203) will increase coverage by 0.20%.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff              @@
##                4.0    #2661      +/-   ##
============================================
+ Coverage     90.99%   91.20%   +0.20%     
  Complexity      757      757              
============================================
  Files            35       35              
  Lines          1933     1933              
============================================
+ Hits           1759     1763       +4     
+ Misses          174      170       -4

Files	Coverage Δ
src/Eloquent/Builder.php	`89.33% <ø> (+2.66%)`	⬆️

... and 1 file with indirect coverage changes

Treggats · 2023-11-02T16:18:38Z

src/Eloquent/Builder.php

        'max',
        'min',
        'pluck',
        'pull',
        'push',
        'raw',
        'sum',
-        'toSql',
+        'tomql',
+        // Kept for compatibility with Laravel < 10.3


And here I was trying to conditionally set the method names casing 😆

Simple and verbose solutions are often good enough.

GromNaN

Thank you @Treggats for the time you've devoted to correcting the breaking changes coming from Laravel.

Treggats · 2023-11-02T16:21:55Z

Thank you @Treggats for the time you've devoted to correcting the breaking changes coming from Laravel.

@GromNaN You're welcome, we had to pin our laravel/framework version because of it. So we needed a fix for this.

GromNaN · 2023-11-02T16:27:25Z

I'm going to make a release tomorrow.

Treggats mentioned this pull request Nov 1, 2023

model id property returns the builder and not the id #2660

Closed

GromNaN reviewed Nov 2, 2023

View reviewed changes

src/Eloquent/Builder.php Outdated Show resolved Hide resolved

GromNaN reviewed Nov 2, 2023

View reviewed changes

Treggats requested a review from GromNaN November 2, 2023 13:30

Treggats commented Nov 2, 2023

View reviewed changes

GromNaN mentioned this pull request Nov 2, 2023

Test with lowest dependencies in CI #2663

Merged

GromNaN reviewed Nov 2, 2023

View reviewed changes

tests/Eloquent/CallBuilderTest.php

];

yield 'dd' => [

'dd',

Copy link

Member

GromNaN Nov 2, 2023

Choose a reason for hiding this comment

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

We can create a separate issue to support this, once this PR get merged.

This comment was marked as outdated.

Sign in to view

GromNaN reviewed Nov 2, 2023

View reviewed changes

tests/Eloquent/CallBuilderTest.php Outdated Show resolved Hide resolved

GromNaN force-pushed the sync-casing-with-base-model branch 4 times, most recently from f80b7ac to abcba92 Compare November 2, 2023 16:16

GromNaN changed the base branch from 4.1 to 4.0 November 2, 2023 16:17

Treggats commented Nov 2, 2023

View reviewed changes

lowercase the $passthru array values

a14a822

GromNaN force-pushed the sync-casing-with-base-model branch from abcba92 to a14a822 Compare November 2, 2023 16:19

GromNaN approved these changes Nov 2, 2023

View reviewed changes

GromNaN merged commit bd6f1c3 into mongodb:4.0 Nov 2, 2023
13 checks passed

Treggats deleted the sync-casing-with-base-model branch November 2, 2023 16:23

GromNaN changed the title ~~lowercase the $passthru array values~~ Fix compatibility with Laravel 10.30 — lowercase the $passthru array values Nov 2, 2023

GromNaN modified the milestones: 4.0.1, 4.0 Nov 2, 2023

GromNaN mentioned this pull request Nov 3, 2023

Upgrade minimum Laravel version to 10.30 #2665

Merged

willtj mentioned this pull request Jul 3, 2024

fix: Laravel 10 compatibility trybeapp/laravel-mongodb#5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with Laravel 10.30 — lowercase the `$passthru` array values #2661

Fix compatibility with Laravel 10.30 — lowercase the `$passthru` array values #2661

Treggats commented Nov 1, 2023 •

edited

Loading

GromNaN left a comment

GromNaN Nov 2, 2023 •

edited

Loading

Treggats Nov 2, 2023

Treggats commented Nov 2, 2023

Treggats commented Nov 2, 2023

GromNaN commented Nov 2, 2023 •

edited

Loading

Treggats commented Nov 2, 2023

hans-thomas commented Nov 2, 2023

Treggats commented Nov 2, 2023

Treggats Nov 2, 2023

Treggats Nov 2, 2023

Treggats Nov 2, 2023

Treggats commented Nov 2, 2023

GromNaN commented Nov 2, 2023

GromNaN Nov 2, 2023

This comment was marked as outdated.

codecov-commenter commented Nov 2, 2023 •

edited

Loading

Treggats Nov 2, 2023

GromNaN Nov 2, 2023

GromNaN left a comment

Treggats commented Nov 2, 2023

GromNaN commented Nov 2, 2023

Fix compatibility with Laravel 10.30 — lowercase the $passthru array values #2661

Fix compatibility with Laravel 10.30 — lowercase the $passthru array values #2661

Conversation

Treggats commented Nov 1, 2023 • edited Loading

GromNaN left a comment

Choose a reason for hiding this comment

GromNaN Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Treggats Nov 2, 2023

Choose a reason for hiding this comment

Treggats commented Nov 2, 2023

Treggats commented Nov 2, 2023

GromNaN commented Nov 2, 2023 • edited Loading

Treggats commented Nov 2, 2023

hans-thomas commented Nov 2, 2023

Treggats commented Nov 2, 2023

Treggats Nov 2, 2023

Choose a reason for hiding this comment

Treggats Nov 2, 2023

Choose a reason for hiding this comment

Treggats Nov 2, 2023

Choose a reason for hiding this comment

Treggats commented Nov 2, 2023

GromNaN commented Nov 2, 2023

GromNaN Nov 2, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

codecov-commenter commented Nov 2, 2023 • edited Loading

Codecov Report

Treggats Nov 2, 2023

Choose a reason for hiding this comment

GromNaN Nov 2, 2023

Choose a reason for hiding this comment

GromNaN left a comment

Choose a reason for hiding this comment

Treggats commented Nov 2, 2023

GromNaN commented Nov 2, 2023

Fix compatibility with Laravel 10.30 — lowercase the `$passthru` array values #2661

Fix compatibility with Laravel 10.30 — lowercase the `$passthru` array values #2661

Treggats commented Nov 1, 2023 •

edited

Loading

GromNaN Nov 2, 2023 •

edited

Loading

GromNaN commented Nov 2, 2023 •

edited

Loading

codecov-commenter commented Nov 2, 2023 •

edited

Loading