kirschbaum-development / eloquent-power-joins

The Laravel magic you know, now applied to joins.
MIT License
1.41k stars 87 forks source link

Faulty default `WHERE` clause & `joinRelationship()` on morphable `MorphTo` has errors #160

Closed vHeemstra closed 1 year ago

vHeemstra commented 1 year ago

Consider the set-up as in issue #147

If you run:

OrderItem::joinRelationship('orderable', morphable: Product::class)->getQuery()->toSQL()

It throws a deprecation error (using PHP 8.1.10):

DEPRECATED  str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in vendor\laravel\framework\src\Illuminate\Databas
e\Eloquent\Model.php on line 569.

This is because the column variable, that is being checked there, is null instead of a proper name.

I suspect this empty column is from a (faulty) WHERE clause that is present by default on the relation's base query:

OrderItem::getModel()->orderable()->getBaseQuery()->wheres
/*
  Returns:
  [
    [
      "type" => "Null",
      "column" => "order_items.",
      "boolean" => "and",
    ],
  ]
*/
vHeemstra commented 1 year ago

I'm not sure what produces this weird WHERE clause. Is it a Laravel default or a side-effect from this package?

luisdalmolin commented 1 year ago

I cannot replicate this. Could you post the models here for reference?

vHeemstra commented 1 year ago

I cannot replicate this. Could you post the models here for reference?

Yes, I created a repo for my test project: https://github.com/vHeemstra/powerjoins-playground

I'm using: Laravel v10.29.0
PHP v8.1.10 (the failed test at #161 used v8.0.30 and aborted the other tests) Composer v2.4.4 eloquent-power-joins v3.3.0

In the README there I put some additional information and steps to reproduce.

luisdalmolin commented 1 year ago

I pushed a new version with the fix for this. I tested with your test repo and it doesn't throw the deprecated error anymore.

luisdalmolin commented 1 year ago

Thanks for the report and the reproducible repo.

vHeemstra commented 1 year ago

No problem! Glad to help.

Did you also read the last paragraph of the repo readme? Is that indeed another bug? (About the deleted_at column.)

luisdalmolin commented 1 year ago

Yep, that was also part of the bug.

vHeemstra commented 1 year ago

Awesome, thanks 👍