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

think I found a bug with where("something", "somevalue") and where("something", "<>", "somevalue") #62

Closed
vesper8 opened this issue Jan 20, 2018 · 10 comments
Assignees
Labels

Comments

@vesper8
Copy link
Contributor

vesper8 commented Jan 20, 2018

One part of my application seems to be broken since I started using this package. I've isolated the problem now

I have a table with 300 records

$records = Place::where('place_id', $placeId)->get();

$records->count() is = 1 here

then below I run

$records = Place::where('place_id', '!=', $placeId)->get();

I tried with '<>' but that is the same as '!='

$records->count() should be 299 now, but instead it uses the cache from the above query and returns the same as the above query.. so count is 1.. and instead of getting "Every other record" it is just returning the same record

So it looks like it doesn't differentiate between the two

@mikebronner
Copy link
Owner

Thanks @vesper8 ! I will look into that as soon as possible (I'm currently out of town and will return next week). It looks like I need to differentiate the where clauses better. Thanks so much for reporting this. :)

@mikebronner mikebronner self-assigned this Jan 25, 2018
@vmosoti
Copy link

vmosoti commented Jan 25, 2018

seems like a bug when using where with other operators including <,>,<=,>=

@mikebronner
Copy link
Owner

yea, that's my thinking as well

@vesper8
Copy link
Contributor Author

vesper8 commented Feb 6, 2018

this is a pretty dangerous bug that's easy to miss and can cause a lot of problems.. hope you can fix it soon :)

@mikebronner
Copy link
Owner

Yea, sorry about the delay ... I have been out of the office most of the last month. Will try to get to it this week. :|

@mikebronner
Copy link
Owner

@vesper8 Thanks for being patient. Can you verify that 0.2.19 fixes this issue? Thanks! :)

@vesper8
Copy link
Contributor Author

vesper8 commented Feb 8, 2018

@mikebronner thanks for making the changes!

I will check it out.. just as soon as I figure out the best way to merge your changes into my fork

But actually I would much prefer not to have to use my fork

Can you have a look at my two commits in my fork here
master...vesper8:master

And tell me if you would be willing to accept a pull request for these changes?

Basically I need these two changes to make your package compatible with Mongodb models

I am quite certain this does not break anything

@mikebronner
Copy link
Owner

mikebronner commented Feb 9, 2018

@vesper8 Ah yes, those are on the docket for review. Can you you submit those as a PR, then we can discuss them there? That would be awesome!

@vesper8
Copy link
Contributor Author

vesper8 commented Feb 18, 2018

@mikebronner I just made a PR for one of the changes

As for the other.. I'm confused.. I tried to "blame" on the CachedKey.php but I can't figure out in which commit you removed the line


$value .= "_" . array_get($where, 'value');

which is last shown in this commit e150f28

Since this is the line that I needed to follow up with

 if (is_array($value)) {
     $value = implode("_", $value);
 }

Which could be more elegantly adapted to

$value .= "_" . (
is_array(array_get($where, 'value')) ? implode("_", array_get($where, 'value')) : array_get($where, 'value'))

or maybe you can come up with something even nicer

@mikebronner
Copy link
Owner

Hi @vesper8 , that line was removed a few releases ago. The functionality has been moved into the getValues (or similar, I'm not at my computer right now) method further down. I plan on redoing how the keys are built up anyway, so it likely won't matter anymore then.

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

No branches or pull requests

3 participants