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

Larave 5.4 #159

Closed
divdax opened this issue Jan 31, 2017 · 32 comments
Closed

Larave 5.4 #159

divdax opened this issue Jan 31, 2017 · 32 comments

Comments

@divdax
Copy link

divdax commented Jan 31, 2017

I think we need an update. 😄

@nospoon
Copy link

nospoon commented Feb 7, 2017

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

@php3ch0
Copy link

php3ch0 commented Feb 15, 2017

+1

@redroses
Copy link

Agreed. Looks like just an update to composer.json

@Sin30
Copy link

Sin30 commented Feb 17, 2017

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

@linxun
Copy link

linxun commented Feb 17, 2017

+1

2 similar comments
@mjurkowski
Copy link

+1

@maig81
Copy link

maig81 commented Feb 21, 2017

+1

@nospoon
Copy link

nospoon commented Feb 21, 2017

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

@maxvaser
Copy link

maxvaser commented Feb 21, 2017

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

@nospoon
Copy link

nospoon commented Feb 22, 2017

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
Copy link

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

@nospoon
Copy link

nospoon commented Feb 22, 2017

@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 (#155)

@AdrienPoupa
Copy link

I think sofa/hookable should be updated as well.

@Remo
Copy link

Remo commented Feb 26, 2017

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
Copy link

Remo commented Mar 6, 2017

It seems like there's a problem now!
I'm running into this issue #21

@Remo
Copy link

Remo commented Mar 6, 2017

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
ortic@b9ddde9

@Remo
Copy link

Remo commented Mar 6, 2017

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
Copy link

php-rock commented Mar 6, 2017

+1

3 similar comments
@lchogan
Copy link

lchogan commented Mar 8, 2017

+1

@TimoStahl
Copy link

+1

@dermatzeimnetz
Copy link

+1

@Remo
Copy link

Remo commented Mar 16, 2017

it's not that simple, here's another issue we would have to take care of #166

@dv336699
Copy link

dv336699 commented Mar 20, 2017

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
Copy link

Remo commented Mar 21, 2017

@diego-vieira this should fix the failure ortic#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
Copy link

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

@ebbbang
Copy link

ebbbang commented Mar 21, 2017

@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
Copy link

Remo commented Mar 21, 2017

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

@jarektkaczyk
Copy link
Owner

jarektkaczyk commented Mar 21, 2017 via email

@ebbbang
Copy link

ebbbang commented Mar 21, 2017

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
Copy link

maxvaser commented Apr 21, 2017

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
Copy link
Owner

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests