tjyoung
asked on
Trying to add conditional 'where' clause into query builder laravel
Hi,
I have a where clause I'm trying to insert that is conditional. I get a syntax error: "unexpected 'if' (T_IF)".
Not sure how to write this?
I have a where clause I'm trying to insert that is conditional. I get a syntax error: "unexpected 'if' (T_IF)".
Not sure how to write this?
$vehicles = DB::table('used_vehicles')
->select('used_vehicles.id','used_vehicles.dealer_id','used_vehicles.category_id','used_vehicles.make_id','used_vehicles.model_id','used_vehicles.construction_year','used_vehicles.cab_style','used_vehicles.stock_number','used_vehicles.vehicle_price','used_vehicles.discount','used_vehicles.vehicle_price - used_vehicles.discount AS used_vehicles.vehicle_price','dealerships.name','makes.name as make_name','models.model_name as model_name','used_vehicles_categories.category_title as category_name')
->join('dealerships', 'used_vehicles.dealer_id', '=', 'dealerships.id')
->join('makes', 'used_vehicles.make_id', '=', 'makes.id')
->join('models', 'used_vehicles.model_id', '=', 'models.id')
->join('used_vehicles_categories', 'used_vehicles.category_id', '=', 'used_vehicles_categories.id')
if (isset(Auth::user()->dealership->name))
{
$vehicles->where('used_vehicles.dealer_id', Auth::user()->dealership_id);
}
->where('used_vehicles.archived', 0)
->orderBy( 'added', 'DESC')
->get();
I would write it in two completely separate calls to the DB::table() facade. Putting the if() statement into the middle of the chained methods seems like a logically good idea, but it bumps into PHP syntax. Maybe something like this (I don't have a Laravel instance to test, but it seems OK).
/**
* IF THE DEALERSHIP IS LOGGED IN
*/
if (isset(Auth::user()->dealership->name))
{
$vehicles = DB::table('used_vehicles')
->select('used_vehicles.id','used_vehicles.dealer_id','used_vehicles.category_id','used_vehicles.make_id','used_vehicles.model_id','used_vehicles.construction_year','used_vehicles.cab_style','used_vehicles.stock_number','used_vehicles.vehicle_price','used_vehicles.discount','used_vehicles.vehicle_price - used_vehicles.discount AS used_vehicles.vehicle_price','dealerships.name','makes.name as make_name','models.model_name as model_name','used_vehicles_categories.category_title as category_name')
->join('dealerships', 'used_vehicles.dealer_id', '=', 'dealerships.id')
->join('makes', 'used_vehicles.make_id', '=', 'makes.id')
->join('models', 'used_vehicles.model_id', '=', 'models.id')
->join('used_vehicles_categories', 'used_vehicles.category_id', '=', 'used_vehicles_categories.id')
// ADDITIONAL WHERE CLAUSE TO RESTRICT RESULTS ACCORDING TO DEALERSHIP ID
->where('used_vehicles.dealer_id', Auth::user()->dealership_id)
->where('used_vehicles.archived', 0)
->orderBy( 'added', 'DESC')
->get();
}
/**
* IF THE DEALERSHIP IS _NOT_ LOGGED IN
*/
else
{
$vehicles = DB::table('used_vehicles')
->select('used_vehicles.id','used_vehicles.dealer_id','used_vehicles.category_id','used_vehicles.make_id','used_vehicles.model_id','used_vehicles.construction_year','used_vehicles.cab_style','used_vehicles.stock_number','used_vehicles.vehicle_price','used_vehicles.discount','used_vehicles.vehicle_price - used_vehicles.discount AS used_vehicles.vehicle_price','dealerships.name','makes.name as make_name','models.model_name as model_name','used_vehicles_categories.category_title as category_name')
->join('dealerships', 'used_vehicles.dealer_id', '=', 'dealerships.id')
->join('makes', 'used_vehicles.make_id', '=', 'makes.id')
->join('models', 'used_vehicles.model_id', '=', 'models.id')
->join('used_vehicles_categories', 'used_vehicles.category_id', '=', 'used_vehicles_categories.id')
// NO ADDITIONAL WHERE CLAUSE
->where('used_vehicles.archived', 0)
->orderBy( 'added', 'DESC')
->get();
}
@Ray - I see where you are going and under less complex conditions I would agree with you - however I see a potential for time bomb in your implementation.
If the structure of the query changes in the future - you now have two points of change to the query logic - forget to update one or update it incorrectly and you have inconsistent results.
In this case the two conditions are mostly the same - so I would keep like code common and simply add the where in based on the condition.
If the structure of the query changes in the future - you now have two points of change to the query logic - forget to update one or update it incorrectly and you have inconsistent results.
In this case the two conditions are mostly the same - so I would keep like code common and simply add the where in based on the condition.
@Julian: I certainly take your point. I was thinking about this as I was writing the if() statement. If() adds an element of cyclomatic complexity that makes it necessary to keep the unit tests organized in parallel, too. I was assuming, maybe incorrectly, that this would be the only place in the script that sets $vehicles. If so, a single if() doesn't seem too problematic. If this is not the only code point to set $vehicles, it certainly argues for refactoring this DB call out to a common code block.
My instinct would be to keep the logged-in experience completely separate from the not-logged-in experience. But without seeing the rest of the application, I'm not sure whether the addition of an if() statement represents a step toward spaghetti code, or just a blip in an otherwise well-structured application.
It's not 100% clear to me whether the order of chained methods matters to the execution of the query. Would be nice if we had that freedom of movement!
https://laravel.com/docs/5.1/queries
My instinct would be to keep the logged-in experience completely separate from the not-logged-in experience. But without seeing the rest of the application, I'm not sure whether the addition of an if() statement represents a step toward spaghetti code, or just a blip in an otherwise well-structured application.
It's not 100% clear to me whether the order of chained methods matters to the execution of the query. Would be nice if we had that freedom of movement!
https://laravel.com/docs/5.1/queries
Julian answer is good, he just forget 2 lines before the get()
if you get a lot of these conditional, it may be worth to write a scope function to the UsedVehicles Model.
which you can use in the query chain as:
...->authDealer()->...
if you get a lot of these conditional, it may be worth to write a scope function to the UsedVehicles Model.
function scopeAuthDealer($query) {
if (isset(Auth::user()->dealership->name)) {
$query->where('dealer_id', Auth::user()->dealership_id);
}
return $query;
}
which you can use in the query chain as:
...->authDealer()->...
ASKER CERTIFIED SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
ASKER
Hi,
Trying with Julian's code above but getting:
FatalErrorException in InventoryController.php line 30: syntax error, unexpected '$whereClause' (T_VARIABLE)
My exact query:
Trying with Julian's code above but getting:
FatalErrorException in InventoryController.php line 30: syntax error, unexpected '$whereClause' (T_VARIABLE)
My exact query:
public function readAll(ReadAllInventoryRequest $request)
{
$vehicles = DB::table('used_vehicles')
$whereClause = array(
'used_vehicles.archived' => 0,
);
if (isset(Auth::user()->dealership->name)) {
$whereClause['used_vehicles.dealer_id'] = Auth::user()->dealership_id;
}
->select('used_vehicles.id','used_vehicles.dealer_id','used_vehicles.category_id','used_vehicles.make_id','used_vehicles.model_id','used_vehicles.construction_year','used_vehicles.cab_style','used_vehicles.stock_number','used_vehicles.vehicle_price','used_vehicles.discount','used_vehicles.vehicle_price - used_vehicles.discount AS used_vehicles.vehicle_price','dealerships.name','makes.name as make_name','models.model_name as model_name','used_vehicles_categories.category_title as category_name')
->join('dealerships', 'used_vehicles.dealer_id', '=', 'dealerships.id')
->join('makes', 'used_vehicles.make_id', '=', 'makes.id')
->join('models', 'used_vehicles.model_id', '=', 'models.id')
->join('used_vehicles_categories', 'used_vehicles.category_id', '=', 'used_vehicles_categories.id')
->where($whereClause)
->orderBy( 'added', 'DESC')
->get();
return response()->json($vehicles, 200);
}
ASKER
Hi,
Just moved the condition before the query and all is fine:
Just moved the condition before the query and all is fine:
public function readAll(ReadAllInventoryRequest $request)
{
$whereClause = array(
'used_vehicles.archived' => 0,
);
if (isset(Auth::user()->dealership->name)) {
$whereClause['used_vehicles.dealer_id'] = Auth::user()->dealership_id;
}
$vehicles = DB::table('used_vehicles')
->select('used_vehicles.id','used_vehicles.dealer_id','used_vehicles.category_id','used_vehicles.make_id','used_vehicles.model_id','used_vehicles.construction_year','used_vehicles.cab_style','used_vehicles.stock_number','used_vehicles.vehicle_price','used_vehicles.discount','used_vehicles.vehicle_price - used_vehicles.discount AS used_vehicles.vehicle_price','dealerships.name','makes.name as make_name','models.model_name as model_name','used_vehicles_categories.category_title as category_name')
->join('dealerships', 'used_vehicles.dealer_id', '=', 'dealerships.id')
->join('makes', 'used_vehicles.make_id', '=', 'makes.id')
->join('models', 'used_vehicles.model_id', '=', 'models.id')
->join('used_vehicles_categories', 'used_vehicles.category_id', '=', 'used_vehicles_categories.id')
->where($whereClause)
->orderBy( 'added', 'DESC')
->get();
return response()->json($vehicles, 200);
}
Apologies - typo on my part - cut and paste was supposed to go before the chain. EE not the best code editor ...
Open in new window