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

Laravel 6 support #74

Closed
rolandsbirons opened this issue Sep 12, 2019 · 28 comments
Closed

Laravel 6 support #74

rolandsbirons opened this issue Sep 12, 2019 · 28 comments
Assignees

Comments

@rolandsbirons
Copy link

Is there any ETA for Laravel 6 support? And will there be Lumen support also?

@tof06
Copy link
Contributor

tof06 commented Sep 16, 2019

I tried to update this package for L6 support.
It seems working, but some tests are not successful (every tests that need to DELETE something in the cache). I don't know why. Therefore, I didn't make a PR yet.
If I use a lada-cache:flush command in a L6 application, it works. In phpunit, it doesn't.

If someone wants to have a look, see https://github.com/tof06/lada-cache/tree/upgrade-tests-and-laravel-6

@spiritix
Copy link
Owner

The plan is definitely to make this package support Laravel 6. Lumen would need to be validated separately, it's possible that it would require larger adaptions. Unfortunately, I don't have resources currently for open source projects, therefore any pull requests are appreciated.

@spiritix
Copy link
Owner

@tof06 After having a very quick look, your branch seems to look good. However, I am pretty sure you will need to update all classes which hook into Eloquent or Database and partially even replace parts of them. They can be found here: https://github.com/spiritix/lada-cache/tree/master/src/Spiritix/LadaCache/Database

@f-liva
Copy link

f-liva commented Nov 18, 2019

Any news here?

@spiritix
Copy link
Owner

I don't have resources at the moment to provide a tested version for Laravel 6, as it seems to involve larger changes to the DBA. Would appreciate if @tof06 could continue working on his draft, will be happy to review and merge it.

@Detrol
Copy link

Detrol commented Dec 8, 2019

I know this is on hold at the moment, but i still want to raise my hand in the matter if that is any help at all. This is one of the best packages i have used. It was a sad day when i upgraded to 6+ and noticed this package didn't work anymore. So for anyone working on this, know that people like me are really looking forward to an update! :) I wish i could help myself, but i am not that good in the deep development part.
@tof06 , did you have any progress on your part?

@tof06
Copy link
Contributor

tof06 commented Dec 9, 2019

Sorry, no progress for me. Don't really had time to look at this.
Anyway, I'm using my own fork on a Laravel 6 project (not yet in production though) and it seems to work well. Queries are cached and fetched from cache when needed. Cache is invalidated on update and delete.
The problem rises when I run unit tests on the package. Cache delete tests are not passing (because keys are not deleted from cache), and I don't really now why.

I also looked at the Database folder, but I'm not sure what to change there. I added the changes in *Connection classes, but it won't solve the flush problem :)

@spiritix
Copy link
Owner

spiritix commented Dec 9, 2019

So since it seems like there is a demand for L6 support, I will try to allocate some resources, hopefully this year. Will keep you guys posted.

@rbruhn
Copy link

rbruhn commented Dec 9, 2019

Well, I gave it a run this morning but I can't even get the tests to run correctly. If anyone knows a good tutorial on how to test Laravel packages, please point me to it.

@spiritix
Copy link
Owner

spiritix commented Dec 9, 2019

@rbruhn What kind of issues are you experiencing?

@rbruhn
Copy link

rbruhn commented Dec 9, 2019

I think it's related to my set up. The paths and stuff.
I forked what tof06 created and got it installed in a new Laravel project. When I run
phpunit vendor/spiritix/lada-cache/tests
It gives me the error: Class 'Spiritix\LadaCache\Tests\Database\Models\Car' not found

I've tried various paths in the autoload-dev:
"Spiritix\LadaCache\Tests\": "vendor/spiritix/lada-cache/tests/"

I just thought I'd take a crack at it but getting these paths to work is beyond me!!

@rbruhn
Copy link

rbruhn commented Dec 9, 2019

And now I just fixed it with
"Spiritix\": "vendor/spiritix/"

@rbruhn
Copy link

rbruhn commented Dec 9, 2019

Update: From what I can tell, it appears the Redis command for deleting keys isn't working. Most of the tests fail because of this, I think. I haven't been able to determine why. I decided to narrow things down and simply run the FlushCommandtest. This creates two cache entries:

vagrant@homestead:~$ redis-cli keys "*"
1) "laravel_database_lada:tag"
2) "laravel_database_lada:key"

I confirmed the keys are retrieved correctly, they are looped over, and the $this->redis->del($key); command is run. However, the keys are never deleted. As I understand it, the command calls the RedisFacade __call and passes it on to Laravel to handle.

I checked if the Redis command line worked: redis-cli del "laravel_database_lada:tag"
It did. So, I dug deeper and ended up in Illuminate\Redis\Connections\Connection.php line 111:

$result = $this->client->{$method}(...$parameters);

This command runs but does not delete the given cache key. I'm at a loss now. I'm running Redis 4.0.9 if that matters.

EDIT: Just to be clear, I did check that the $parameters value matched the existing keys

@tof06
Copy link
Contributor

tof06 commented Dec 10, 2019

@rbruhn So, you got exactly the same behavior than me.

What I don't understand, is that if I use the same code (my fork) in an existing Laravel 6 application, the flush works well:

Before:

127.0.0.1:6379> keys lada*
  1) "lada:6adbb7d01ede449b8343d6fc488445d1"
  2) "lada:72e706d4cd8ba12926b3d8d287261978"
  3) "lada:8265721923acfc3ed303f8aad09c83b4"
  4) "lada:7f73f2622ca7dca7df3898f2d58ad4e7"
  5) "lada:07822773946316e3ba2670d52417a1e8"
  6) "lada:65bf1abd5eebaf1f975f8b56695da1b3"
  7) "lada:9283c7e013090a5c0c030f9bd61dc19e"
  8) "lada:f5e8307dfa6c4f5fa6e8f3d4718f42e3"
  9) "lada:tags:database:WhiteLabel:table_unspecific:domains"
 10) "lada:2fd4513bc5f1071d1ca3267c518ec7f4"
[.....]
$ php artisan lada-cache:flush
**Cache flushed

Then:

127.0.0.1:6379> keys lada*
(empty list or set)

So, the problem raises only in phpunit 😞
I'm not an expert in testing (I know, I should 😄), so, maybe I missed something.

@f-liva
Copy link

f-liva commented Dec 10, 2019 via email

@rbruhn
Copy link

rbruhn commented Dec 10, 2019

Created pull request if someone can check my work. #79

@spiritix
Copy link
Owner

@rbruhn Thanks a lot. Going to check it this week.

@rbruhn
Copy link

rbruhn commented Dec 11, 2019

@spiritix I was curious and decided to run tests with this fix using Laravel 5.8
The tests failed. So I will try to figure that out.

Edit: I need more coffee. Obviously this fix wouldn't work on 5.8. That's the whole point of upgrading it. Please ignore this message.

@spiritix
Copy link
Owner

I could release a new major version for L6 support, shouldn't be a problem.

@Detrol
Copy link

Detrol commented Dec 11, 2019

You guys are truly awesome! You deserve a medal. Thank you for your effort in this! Looking forward to updates :)

@tof06
Copy link
Contributor

tof06 commented Dec 12, 2019

@rbruhn @spiritix Did the PR is running into travis CI ?
I've updated my tree with the patch, add latest updates from master (and resolve conflicts), but travis won't run.. It complains about Redis class not found ???
If I run phpunit on my own php environment, everything's ok.

image

I think I should really learn all this testing & CI stuff 😀

@rbruhn
Copy link

rbruhn commented Dec 12, 2019

Does the travis have php-redis installed? That might be the issue.

@spiritix spiritix self-assigned this Dec 13, 2019
@spiritix
Copy link
Owner

I've now consolidated all pull requests, ideas, and inputs for Laravel 6 support into a new branch (https://github.com/spiritix/lada-cache/tree/74-laravel6-support) and did some updates as well.
The tests are passing, currently working in getting Travis running again. I would appreciate it if you guys could help with testing. Pull the branch, run some crazy queries and let me know if there are any errors, misses or outdated data is being returned. Also, have a look at the output of Laravel Debugbar (https://github.com/barryvdh/laravel-debugbar) to check if the queries are tagged properly.

@Detrol
Copy link

Detrol commented Dec 13, 2019

Is it possible to use a tree via composer require? I would gladly test it in my environment!

@spiritix
Copy link
Owner

Sure, this should work:
composer require spiritix/lada-cache:dev-74-laravel6-support

@Detrol
Copy link

Detrol commented Dec 14, 2019

From what i can see, i have no errors, and most of my queries are going through lada-cache. Although Debugbar is not picking up in the Lada tab. Only in the queries tab.

@rbruhn
Copy link

rbruhn commented Dec 14, 2019

I'm not able to check this for at least a week. I have a project finishing up in the coming week before I can spend time upgrading my app to Laravel 6.

@spiritix
Copy link
Owner

I was able to perform some comprehensive tests now. After all, the cache seems working. Also, the Debug Bar works fine for me. There are some queries that bypass the cache, but that's another topic and should not be part of the L6 migration. I am going to publish a release for L6 now. Thanks for your contribution guys and I would appreciate if you test the new version and report any issues.

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

No branches or pull requests

6 participants