jarektkaczyk / eloquence

Extensions for the Eloquent ORM
http://softonsofa.com
MIT License
1.09k stars 141 forks source link

Larave 5.4 #159

Closed divdax closed 7 years ago

divdax commented 7 years ago

I think we need an update. :smile:

nospoon commented 7 years ago

+1 Any chance we could have it for 5.4 please?

php3ch0 commented 7 years ago

+1

redroses commented 7 years ago

Agreed. Looks like just an update to composer.json

Sin30 commented 7 years ago

@redroses if that's the case, it should be fairly easy to update.

linxun commented 7 years ago

+1

mjurkowski commented 7 years ago

+1

maig81 commented 7 years ago

+1

nospoon commented 7 years ago

It looks like the composer.json was updated quite a while ago to accept illuminate/database 5.4 (https://github.com/jarektkaczyk/eloquence/pull/152), but it seems that the composer repository has not been updated and it is still only accepts 5.3.

maxvaser commented 7 years ago

+1 - This is a great library! Looks like only a minor update like @nospoon mentions to get it working for 5.4

nospoon commented 7 years ago

Just noticed, there is already a pull request waiting to be merged to fix the version requirement. Not sure why it's been sitting there for 28 days now.

aaronsnyder commented 7 years ago

+1 - @nospoon It looks like this change was merged, no? However, still the same result, unable to resolve illuminate\database dependency.

nospoon commented 7 years ago

@aaronsnyder The original PR to introduce 5.4 was indeed merged, however it contained an error (single | instead of ||), there is a second PR to correct that but this one still hasn't been merged (https://github.com/jarektkaczyk/eloquence/pull/155)

AdrienPoupa commented 7 years ago

I think sofa/hookable should be updated as well.

Remo commented 7 years ago

I had to create a fork for this, you can use "ortic/eloquence": "~5.4" till this gets merged. I will happily delete my fork once this has been fixed.

Remo commented 7 years ago

It seems like there's a problem now! I'm running into this issue https://github.com/jarektkaczyk/eloquence/issues/21

Remo commented 7 years ago

I have no clue what I'm doing there, not enough experience regarding eloquent bindings, but I noticed that the values weren't passed into the query, this commit seems to fix things for me. I'm sure there's a better solution though https://github.com/ortic/eloquence/commit/b9ddde95d4bb3692233687ef84cb7f3a2dc90d70

Remo commented 7 years ago

Sorry guys, this patch doesn't work. It's fine as long as you just call the search method, but it fails if you want to filter for another field.

php-rock commented 7 years ago

+1

lchogan commented 7 years ago

+1

TimoStahl commented 7 years ago

+1

dermatzeimnetz commented 7 years ago

+1

Remo commented 7 years ago

it's not that simple, here's another issue we would have to take care of https://github.com/jarektkaczyk/eloquence/issues/166

dv336699 commented 7 years ago

those interested on a working 5.4 version should use this repo until we manage to merge with @jarektkaczyk's repo: https://github.com/ortic/eloquence

Currently I have sent a PR with a couple of fixes. Some tests are still failing. If someone could have a look at the tests and try to fix them that'd be great.

These are the failing errors as of now.

PHPUnit 4.5.0 by Sebastian Bergmann and contributors.

Configuration read from eloquence/phpunit.xml

...........................F..........E........................  63 / 152 ( 41%)
.............................................................E. 126 / 152 ( 82%)
..........................

Time: 1.08 seconds, Memory: 14.00MB

There were 2 errors:

1) Sofa\Eloquence\Tests\MappableTest::mapped_join_polymorphic_relation
Mockery\Exception\NoMatchingExpectationException: No matching handler found for Mockery_0_Illuminate_Database_ConnectionInterface::select("select "companies"."name" from "users" left join "companies" on "companies"."brandable_id" = "users"."id" and "brandable_type" = ? limit 1", array(0=>'UserMorph',), true). Either the method was unexpected or its arguments matched no expected argument list for this method

eloquence/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:93
eloquence/vendor/illuminate/database/Query/Builder.php:1718
eloquence/vendor/illuminate/database/Query/Builder.php:1703
eloquence/vendor/illuminate/database/Query/Builder.php:1686
eloquence/vendor/illuminate/database/Query/Builder.php:1673
eloquence/src/Mappable.php:333
eloquence/src/Mappable.php:165
eloquence/src/Mappable.php:139
eloquence/src/Mappable.php:72
eloquence/src/Mappable/Hooks.php:28
eloquence/vendor/ortic/hookable/src/Pipeline.php:84
eloquence/vendor/ortic/hookable/src/Pipeline.php:88
eloquence/vendor/ortic/hookable/src/Hookable.php:245
eloquence/vendor/ortic/hookable/src/Hookable.php:76
eloquence/vendor/ortic/hookable/src/Builder.php:66
eloquence/vendor/ortic/hookable/src/Builder.php:457
eloquence/vendor/illuminate/database/Eloquent/Model.php:1316
eloquence/tests/MappableTest.php:91

2) Sofa\Eloquence\Tests\SearchableBuilderTest::length_aware_pagination
Mockery\Exception\NoMatchingExpectationException: No matching handler found for Mockery_0_Illuminate_Database_ConnectionInterface::select("select count(*) as aggregate from (select `users`.*, max(case when `users`.`last_name` = ? then 150 else 0 end + case when `users`.`last_name` like ? then 50 else 0 end + case when `users`.`last_name` like ? then 10 else 0 end) as relevance from `users` where (`users`.`last_name` like ?) group by `users`.`primary_key`) as `users` where `relevance` >= 2.5", array(), true). Either the method was unexpected or its arguments matched no expected argument list for this method

eloquence/vendor/mockery/mockery/library/Mockery/ExpectationDirector.php:93
eloquence/vendor/illuminate/database/Query/Builder.php:1718
eloquence/vendor/illuminate/database/Query/Builder.php:1703
eloquence/vendor/illuminate/database/Query/Builder.php:1803
eloquence/vendor/illuminate/database/Query/Builder.php:1776
eloquence/vendor/illuminate/database/Eloquent/Builder.php:1332
eloquence/tests/SearchableBuilderTest.php:186

--

There was 1 failure:

1) Sofa\Eloquence\Tests\JoinerTest::it_joins_dot_nested_relations
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'select * from "users" inner join "profiles" on "users"."profile_id" = "profiles"."id" inner join "companies" on "companies"."morphable_id" = "profiles"."id" and "companies"."morphable_type" = ?'
+'select * from "users" inner join "profiles" on "users"."profile_id" = "profiles"."id" inner join "companies" on "companies"."morphable_id" = "profiles"."id" and "morphable_type" = ?'

eloquence/tests/JoinerTest.php:38

FAILURES!
Tests: 152, Assertions: 207, Failures: 1, Errors: 2.
Script phpunit && ./vendor/bin/phpcs src --standard=psr2 --report=diff --colors handling the test event returned with error code 2
Remo commented 7 years ago

@diego-vieira this should fix the failure https://github.com/ortic/eloquence/pull/2 I wonder whether I should set up travis to properly check these things. I'm not a big fan of forking, but better a maintained fork than a broken library. What's the opinion about that here?

dv336699 commented 7 years ago

@Remo indeed, but we can always merge your repo with this one as soon as @jarektkaczyk is back.

ebbbang commented 7 years ago

@Remo Made a pull request that would IMHO pass all Tests and errors ...

Would integrate in my project after your merge my pull request and let you know how it goes ...

Feedback is much appreciated ...

Remo commented 7 years ago

Thanks, check this https://travis-ci.org/ortic/eloquence

jarektkaczyk commented 7 years ago

I will look into it ASAP. Thanks

Jarek Tkaczyk

softonsofa :: development with pleasure

On Mar 21, 2017 19:31, "Ebrahim Bangdiwala" notifications@github.com wrote:

@Remo https://github.com/Remo Made a pull request that would IMHO pass all Tests and errors ...

Would integrate in my project after your pull request and let you know how it goes ...

Feedback is much appreciated ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jarektkaczyk/eloquence/issues/159#issuecomment-288051219, or mute the thread https://github.com/notifications/unsubscribe-auth/AGm5srWtgraFCDcaL73esoDFwb87T4eGks5rn7UBgaJpZM4LyVDx .

ebbbang commented 7 years ago

Hey @jarektkaczyk , I made the PR on @Remo's repo ... We have made a lot of changes. I would request @Remo to create a new PR on @jarektkaczyk's repo so he can check it out.

Thanks to @jarektkaczyk , @Remo , @diego-vieira for the great work ...

maxvaser commented 7 years ago

Hi @jarektkaczyk, have you been able to look at @remo's PR? It's been a month. It would great to get your awesome package working again in Laravel 5.4.

jarektkaczyk commented 7 years ago

upgraded to 5.4 finally ;) https://github.com/jarektkaczyk/eloquence/tree/5.4

thanks to @Remo and everyone else who made effort, sorry it took so long

maxvaser commented 7 years ago

Thanks @jarektkaczyk!!! and for the subsequent quick bug fix!