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

[3.9] Fix/stringable sort #2420

Merged
merged 7 commits into from Sep 1, 2022
Merged

Conversation

apeisa
Copy link
Contributor

@apeisa apeisa commented Jul 16, 2022

This PR replaces an older one. This is a safe fix, where we type cast column value to string when ordering. This is more inline in how the Eloquent method that is overwritten works too.

There is a simple test to show how you can now pass objects that implement Stringable as a $column variable for orderBy method.

I wasn't sure where to add that mockable class, so I did put it directly into QueryTest.php file.

@apeisa
Copy link
Contributor Author

apeisa commented Jul 16, 2022

You can also see all tests passing from my own workflows here https://github.com/apeisa/laravel-mongodb/actions/runs/2681478769

@apeisa
Copy link
Contributor Author

apeisa commented Jul 23, 2022

@divine Is it possible to merge this?

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.

Also, point me where exactly that library converts this to the stringable object.

Thanks!

tests/QueryTest.php Outdated Show resolved Hide resolved
tests/QueryTest.php Outdated Show resolved Hide resolved
apeisa and others added 2 commits July 25, 2022 13:58
Co-authored-by: Divine <48183131+divine@users.noreply.github.com>
Co-authored-by: Divine <48183131+divine@users.noreply.github.com>
@apeisa
Copy link
Contributor Author

apeisa commented Jul 25, 2022

Thanks for the suggestion, didn't know about the str() helper, makes the test much cleaner!

Not sure I understand your request:

Also, point me where exactly that library converts this to the stringable object.

If you are referring to the Filament library - I don't think it matters (this fix is not about Filament only). I mean the Eloquent orderBy method that we are overriding allows multiple objects (in addition to string). Most of them are Queryable-objects, but it also accepts Illuminate\Database\Query\Expression -objects, which are stringable ones.

We could use also $age = new \Illuminate\Database\Query\Expression('age'); where we now have $age = str('age'); to make that point more clear.

This fix simply makes it possible to give any stringable object for orderBy method, instead of only pure strings. Of course, we could check also if the object is Expression, and only then do type cast to a string, but I think code is cleaner and more future-proof this way.

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2022

Codecov Report

Merging #2420 (f670c5f) into master (ad4422a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #2420   +/-   ##
=========================================
  Coverage     87.62%   87.62%           
  Complexity      676      676           
=========================================
  Files            31       31           
  Lines          1551     1551           
=========================================
  Hits           1359     1359           
  Misses          192      192           
Impacted Files Coverage Δ
src/Query/Builder.php 89.64% <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 ad4422a...f670c5f. Read the comment docs.

@divine divine requested a review from Smolevich July 26, 2022 05:37
@apeisa
Copy link
Contributor Author

apeisa commented Sep 1, 2022

@divine @Smolevich any hope to get this merged for the next release?

@Smolevich Smolevich changed the title Fix/stringable sort [3.9] Fix/stringable sort Sep 1, 2022
@Smolevich Smolevich merged commit 5bcc82e into mongodb:master Sep 1, 2022
@apeisa apeisa deleted the fix/stringable-sort branch September 1, 2022 15:00
@apeisa
Copy link
Contributor Author

apeisa commented Sep 1, 2022

Thanks!

divine added a commit that referenced this pull request Jan 16, 2023
commit 551ec9f
Author: Henrique Troiano <63327237+henriquetroiano@users.noreply.github.com>
Date:   Sun Jan 15 20:19:33 2023 -0300

    Add Geonear instructions to ReadMe. Closes #1878 (#2487)

    * create aggregate geonear instructions

    * create aggregate geonear instructions

    * create aggregate geonear instructions

    * create aggregate geonear instructions

    * create aggregate geonear instructions

    * chore: revert back readme changes

    * chore: minor readme change

    Co-authored-by: henriquetroiano <henriquetroiano@hoomweb.com>
    Co-authored-by: divine <48183131+divine@users.noreply.github.com>

commit 317f702
Author: Hikmat <hikmet.hesenov.93@gmail.com>
Date:   Sun Jan 15 04:58:12 2023 -0800

    fix: whereBelongsTo (#2454)

    * override getQualifiedForeignKeyName() and add tests for whereBelongsTo

    * delete unneeded query() call

    * type hint for getQualifiedForeignKeyName()

    Co-authored-by: Hikmat Hasanov <hikmet.hasanov@proton.me>

commit e5e9193
Author: Andreas Braun <alcaeus@users.noreply.github.com>
Date:   Sun Nov 27 19:32:25 2022 +0100

    Transaction support (#2465)

    * Add support for transactions

    Co-authored-by: klinson <klinson@163.com>
    Co-authored-by: levon80999 <levonb@ucraft.com>

    * Start single-member replica set in CI

    Co-authored-by: levon80999 <levonb@ucraft.com>

    * Add connection options for faster failures in tests

    The faster connection and server selection timeouts ensure we don't spend too much time waiting for the inevitable as we're expecting fast connections on CI systems

    Co-authored-by: levon80999 <levonb@ucraft.com>

    * Apply readme code review suggestions

    * Simplify replica set creation in CI

    * Apply feedback from code review

    * Update naming of database env variable in tests

    * Use default argument for server selection (which defaults to primary)

    * Revert "Simplify replica set creation in CI"

    This partially reverts commit 203160e. The simplified call unfortunately breaks tests.

    * Pass connection instance to transactional closure

    This is consistent with the behaviour of the original ManagesTransactions concern.

    * Correctly re-throw exception when callback attempts have been exceeded.

    * Limit transaction lifetime to 5 seconds

    This ensures that hung transactions don't block any subsequent operations for an unnecessary period of time.

    * Add build step to print MongoDB server status

    * Update src/Concerns/ManagesTransactions.php

    Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

    Co-authored-by: klinson <klinson@163.com>
    Co-authored-by: levon80999 <levonb@ucraft.com>
    Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

commit 0606fc0
Author: Andreas Braun <alcaeus@users.noreply.github.com>
Date:   Mon Nov 14 12:51:00 2022 +0100

    Use single connection using DSN for testing (#2462)

    * Always use connection string in tests

    * Document DSN configuration as preferred configuration method

    * Update wordings

    * Use matrix config instead of manually specifying builds

    * Apply StyleCI fixes

    * Add missing test for code coverage

commit 560e05e
Author: Andreas Braun <alcaeus@users.noreply.github.com>
Date:   Mon Nov 14 12:46:55 2022 +0100

    Chore: add types where safe (#2464)

    * Use direct method calls over call_user_func_array

    * Add return types where safely possible

    * Fix styleCI issues

commit dbde512
Author: Andreas Braun <alcaeus@users.noreply.github.com>
Date:   Thu Nov 10 13:55:56 2022 +0100

    Pass timeout in milliseconds (#2461)

commit 42e0100
Author: abofazl rasoli <75317352+abolfazl-rasoli@users.noreply.github.com>
Date:   Fri Nov 4 16:18:52 2022 +0330

    chore: test firstOrCreate method for the model (#2399)

commit 61cc6ed
Author: Stas <smolevich90@gmail.com>
Date:   Thu Sep 1 16:20:31 2022 +0300

    Add info about new release 3.9.2 (#2440)

    * Add info about new release 3.9.2

    * chore: update changelog

    Co-authored-by: Divine <48183131+divine@users.noreply.github.com>

commit 5bcc82e
Merge: 41a9c97 f670c5f
Author: Stas <smolevich90@gmail.com>
Date:   Thu Sep 1 12:58:26 2022 +0300

    Merge pull request #2420 from apeisa/fix/stringable-sort

    [3.9] Fix/stringable sort

commit 41a9c97
Merge: ad4422a c27924c
Author: Stas <smolevich90@gmail.com>
Date:   Thu Sep 1 12:55:15 2022 +0300

    Merge pull request #2438 from RosemaryOrchard/pr/2394

    [3.9] Respect Laravel 9's single word name mutators

commit c27924c
Author: Rosemary Orchard <contact@rosemaryorchard.com>
Date:   Thu Aug 25 17:11:29 2022 +0100

    Add tests for the mutator

commit 55e80a2
Merge: 6b11977 9b3ffc2
Author: Rosemary Orchard <contact@rosemaryorchard.com>
Date:   Thu Aug 25 14:35:46 2022 +0100

    Merge branch 'pr/2394' of github.com:RosemaryOrchard/laravel-mongodb into pr/2394

commit 6b11977
Author: Rosemary Orchard <contact@rosemaryorchard.com>
Date:   Thu Aug 25 14:17:58 2022 +0100

    Fix formatting

    More CS-Fixer formatting, unrelated to the PR

    PSR2?!

commit 9b3ffc2
Merge: e5a8272 ad4422a
Author: Rosemary Orchard <16113535+RosemaryOrchard@users.noreply.github.com>
Date:   Thu Aug 25 14:32:21 2022 +0100

    Merge branch 'jenssegers:master' into pr/2394

commit e5a8272
Author: Rosemary Orchard <contact@rosemaryorchard.com>
Date:   Thu Aug 25 14:25:57 2022 +0100

    PSR2?!

commit 79fad0e
Author: Rosemary Orchard <contact@rosemaryorchard.com>
Date:   Thu Aug 25 14:22:46 2022 +0100

    More CS-Fixer formatting, unrelated to the PR

commit ce693a8
Author: Rosemary Orchard <contact@rosemaryorchard.com>
Date:   Thu Aug 25 14:17:58 2022 +0100

    Fix formatting

commit f670c5f
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Mon Jul 25 13:59:05 2022 +0300

    Update tests/QueryTest.php

    Co-authored-by: Divine <48183131+divine@users.noreply.github.com>

commit f7895bc
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Mon Jul 25 13:58:59 2022 +0300

    Update tests/QueryTest.php

    Co-authored-by: Divine <48183131+divine@users.noreply.github.com>

commit 3a87b28
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Sat Jul 16 13:02:38 2022 +0300

    style code fixes pt. 3

commit ed86610
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Sat Jul 16 12:55:21 2022 +0300

    make the stringable object type safe

commit 50221ef
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Sat Jul 16 12:53:31 2022 +0300

    style code fixes pt. 2

commit 496f8a0
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Sat Jul 16 12:52:29 2022 +0300

    style code fixes

commit f6c9678
Author: Antti Peisa <antti.peisa@gmail.com>
Date:   Sat Jul 16 12:50:09 2022 +0300

    support stringable objects when sorting

commit ad4422a
Merge: f4c448f fc67e04
Author: Stas <smolevich90@gmail.com>
Date:   Wed Jun 29 22:04:13 2022 +0300

    Merge pull request #2365 from jenssegers/update-changelog

    chore: update changelog and prepare release

commit 5d292a2
Author: Pavel Borunov <8665691+mrneatly@users.noreply.github.com>
Date:   Sat May 28 09:57:03 2022 +0300

    Respect new Laravel accessors's approach

    Fix getting a value from a one-word `\Illuminate\Database\Eloquent\Casts\Attribute`-returning accessors

commit fc67e04
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Fri Mar 11 04:14:08 2022 +0300

    chore: update changelog and prepare release

commit f4c448f
Author: Divine <48183131+divine@users.noreply.github.com>
Date:   Mon Mar 7 23:07:22 2022 +0300

    feat: backport support for cursor pagination (#2362)

    Backport #2358 to L9

    Co-Authored-By: Jeroen van de Weerd <info@jeroenvdweerd.nl>

commit 8d601b2
Author: Rob Brain <robjbrain@gmail.com>
Date:   Wed Mar 2 03:45:43 2022 +0700

    Check if failed log storage is disabled in Queue Service Provider (#2357)

    * fix: check if failed log storage is disabled in Queue Service Provider

    Co-authored-by: divine <48183131+divine@users.noreply.github.com>

commit 1e49c5e
Merge: 6cdd309 b6a76f9
Author: Jens Segers <segers.jens@gmail.com>
Date:   Thu Feb 17 11:00:31 2022 +0100

    Merge pull request #2344 from divine/l9

    feat: initial laravel 9 compatibility

commit b6a76f9
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Wed Feb 9 05:03:48 2022 +0300

    fix: apply php-cs-fixer result

commit 20b9d0e
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Wed Feb 9 04:30:38 2022 +0300

    fix: small php-doc comment

commit 8b4c6dc
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Wed Feb 9 04:26:20 2022 +0300

    fix: apply php-cs-fixer results

commit 40846ab
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Wed Feb 9 04:22:39 2022 +0300

    fix: imrprove php-doc comments

commit 42c1ff2
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Wed Feb 9 04:04:43 2022 +0300

    feat: update php-cs-fixer & docker php version

commit 9c2b001
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Wed Feb 9 03:59:33 2022 +0300

    feat: use stable laravel release

commit d02a46c
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Sun Feb 6 05:40:20 2022 +0300

    feat: initial laravel 9 compatibility

commit 6cdd309
Merge: 6aa6ad1 39f940d
Author: Stas <smolevich90@gmail.com>
Date:   Wed Feb 2 02:35:36 2022 +0300

    Merge pull request #2341 from divine/update-changelog

    [3.8] Update changelog

commit 39f940d
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Tue Feb 1 03:43:56 2022 +0300

    chore: add missing dots to readme.

commit 2322a78
Author: divine <48183131+divine@users.noreply.github.com>
Date:   Tue Feb 1 03:42:44 2022 +0300

    chore: update changelog
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.

None yet

4 participants