nature
nature

Reputation: 307

Laravel 5.4: How to get the same result without nesting one foreach inside another?

What I intend is to get the news $new in a certain language if($new_lang->lang_id == $lang_id), then I look in the table of images @foreach($data['images'] as $image) which corresponds to the news and which is the main one @if($image->imageable_id == $new->id && $image->main == '1').

The problem is that when nesting foreachs, the load is very slow, since they are more than 7,000 news and some 70,000 images.

How to get the same result without nesting one foreach inside another?

@foreach($new->langs as $new_lang)
@if($new_lang->lang_id == $lang_id)
    @foreach($data['images'] as $image)
        @if($image->imageable_id == $new->id && $image->main == '1')
            @php
                $mini = substr($image->path, 0, strrpos( $image->path, "/"));
                $name = substr($image->path, strrpos($image->path, '/') + 1);
                $image_mini = $mini.'/mini-'.$name;
            @endphp
            <div class="crop">
                <a href="{{ route('noticia', [$new->id, $new_lang->slug]) }}">{{ HTML::image(file_exists($image_mini)? $image_mini : $image->path, '', array('class' => 'img-responsive ancho_100')) }}</a>
            </div>
        @endif
    @endforeach
@endif
@endforeach

EDITED: added controller and trait

This is the controller:

    use ListNoticias;

public function actualidad(Request $request)
{
    $data['section_id'] = explode(',', '1,2,3');

    $data['ruta'] = 'actualidad';
    $data['title'] = __('header.actualidad');
    $data['num'] = $request->num;
    $url = $request->url();

    $data = $this->listNoticias($data, $url);

    return view('web.actualidad.listado', compact('data'));
}

And the trait:

trait ListNoticias
{
public function listNoticias($data, $url)
{
    $now = date('Y-m-d');
    $time = date('H:i:s');

    (isset($data['num']))? $num = $data['num'] : $num = '15';

    $data['images'] = Image::where('imageable_type', 'App\Models\Noticia')->get();
    $data['sections']  = Section::all();

    $data['noticias'] = Noticia::with('langs')->where('date', '<', $now)
        ->where('active', '1')
        ->whereIn('section_id', $data['section_id'])
        ->orWhere('date', '=', $now)
        ->where('time', '<=', $time)
        ->where('active', '1')
        ->whereIn('section_id', $data['section_id'])
        ->orderBy('date', 'desc')
        ->orderBy('time', 'desc')
        ->get();

    $data['noticias-es'] = [];
    $data['noticias-en'] = [];
    $data['noticias-pt'] = [];

    foreach($data['noticias'] as $row){
        foreach($row->langs as $row_lang) {
            if ($row_lang->lang_id == '1') {
                $data['noticias-es'][] = $row;
            } elseif ($row_lang->lang_id == '2') {
                $data['noticias-en'][] = $row;
            } elseif ($row_lang->lang_id == '3') {
                $data['noticias-pt'][] = $row;
            } else null;

        }
    }

    // Manual paginate
    /*  Get current page form url e.x. &page=1
        Create a new Laravel collection from the array data
        Slice the collection to get the items to display in current page
        Create our paginator and pass it to the view
        set url path for generated links
    */

    $currentPage = LengthAwarePaginator::resolveCurrentPage();

    // ES
    $itemCollection = collect($data['noticias-es']);
    $currentPageItems = $itemCollection->slice(($currentPage * $num) - $num, $num)->all();
    $data['noticias-es'] = new LengthAwarePaginator($currentPageItems , count($itemCollection), $num);
    $data['noticias-es']->setPath($url);

    // EN
    $itemCollection = collect($data['noticias-en']);
    $currentPageItems = $itemCollection->slice(($currentPage * $num) - $num, $num)->all();
    $data['noticias-en'] = new LengthAwarePaginator($currentPageItems , count($itemCollection), $num);
    $data['noticias-en']->setPath($url);

    // PT
    $itemCollection = collect($data['noticias-pt']);
    $currentPageItems = $itemCollection->slice(($currentPage * $num) - $num, $num)->all();
    $data['noticias-pt'] = new LengthAwarePaginator($currentPageItems , count($itemCollection), $num);
    $data['noticias-pt']->setPath($url);

    return $data;

}
}

EDITED 2

The first view is a partial and is included in this view that have another two foreach nestead. This is because I need to display records in threes. I have commented on other includes that are not necessary for the problem:

@foreach ($data['noticias-'.$lang]->chunk(3) as $chunk)

<div class="col-xs-12 col-sm-12 col-md-12 col-lg-12 pad_inf_2">

    @foreach($chunk as $key => $new)
        <div class="col-xs-12 col-sm-12 col-md-4 col-lg-4 pad_der_1">
            <article>
                //@include('web.index.partials.noticia_etiqueta')
                @include('web.actualidad.partials.noticia_image_listado')
                //@include('web.index.partials.noticia_date_section')
                //@include('web.index.partials.noticia_title')
            </article>
        </div>
    @endforeach

</div>
@endforeach

Model Noticia:

class Noticia extends Model
{
protected $fillable = ['section_id', 'active', 'date', 'time', 'author'];

public function images()
{
    return $this->morphMany('App\Models\Image', 'imageable');
}

public function tags()
{
    return $this->morphToMany('App\Models\Tag', 'taggable');
}

public function section()
{
    return $this->belongsTo('App\Models\Section');
}

public function langs()
{
    return $this->hasMany('App\Models\LangNoticia');
}

}

LangNoticia Model:

class LangNoticia extends Model
{
protected $fillable = ['noticia_id', 'lang_id', 'title', 'lead', 'text', 'author_place', 'slug'];

public function noticia()
{
    return $this->belongsTo('App\Models\Noticia');
}
}

EDITED 3: Reduced code in the trait with the advices of abr

Trait reduced:

trait ListNoticias
{
public function listNoticias($data, $url)
{
    $lang = Session::get('lang');
    if($lang == 'en') $lang_id = '2';
    elseif($lang == 'pt') $lang_id = '3';
    else $lang_id = '1';

    $now = date('Y-m-d');
    $time = date('H:i:s');

    (isset($data['num']))? $num = $data['num'] : $num = '15';

    $data['sections']  = Section::all();
    $data['images'] = Image::where('imageable_type', 'App\Models\Noticia')->where('main', true)->get();

    $noticias = Noticia::with('langs')
        ->where('date', '<', $now)
        ->where('active', '1')
        ->whereIn('section_id', $data['section_id'])
        ->orWhere('date', '=', $now)
        ->where('time', '<=', $time)
        ->where('active', '1')
        ->whereIn('section_id', $data['section_id'])
        ->orderBy('date', 'desc')
        ->orderBy('time', 'desc')
        ->get();

    $data['noticias-'.$lang] = [];

    foreach($noticias as $row){
        foreach($row->langs as $row_lang) {
            if ($row_lang->lang_id == $lang_id) {
                $data['noticias-'.$lang][] = $row;
            }
        }
    }

    // Paginate
    /*  Get current page form url e.x. &page=1
        Create a new Laravel collection from the array data
        Slice the collection to get the items to display in current page
        Create our paginator and pass it to the view
        set url path for generated links
    */

    $currentPage = LengthAwarePaginator::resolveCurrentPage();

    $itemCollection = collect($data['noticias-'.$lang]);
    $currentPageItems = $itemCollection->slice(($currentPage * $num) - $num, $num)->all();
    $data['noticias-'.$lang] = new LengthAwarePaginator($currentPageItems , count($itemCollection), $num);
    $data['noticias-'.$lang]->setPath($url);

    return $data;

}
}

Upvotes: 0

Views: 118

Answers (2)

Camilo
Camilo

Reputation: 7194

I think you can access the news main image like this:

$new_lang->noticia->images()->where('main', true)->first();

You could define an accessor in your LangNoticia model to facilitate the call.

public function getImageAttribute()
{
    return $this->noticia->images()->where('main', true)->first();
}

In your view you can remove the foreach() and call the accessor instead.

@foreach($new->langs as $new_lang)
    @if($new_lang->lang_id == $lang_id)
        @php
            $mini = substr($new_lang->image->path, 0, strrpos( $new_lang->image->path, "/"));
            $name = substr($new_lang->image->path, strrpos($new_lang->image->path, '/') + 1);
            $image_mini = $mini.'/mini-'.$name;
        @endphp
        <div class="crop">
            <a href="{{ route('noticia', [$new->id, $new_lang->slug]) }}">{{ HTML::image(file_exists($image_mini)? $image_mini : $new_lang->image->path, '', array('class' => 'img-responsive ancho_100')) }}</a>
        </div>
    @endif
@endforeach

Now you can remove the call to the Image model in your trait.

Use eager loading when querying the Noticia model to reduce the number of queries.

Noticia::with(['images' => function($query) {
    $query->where('main', true);
}])->get();

Upvotes: 1

abr
abr

Reputation: 2129

Just sharing some thoughts on it. Starting with retrieving the content of Noticias:

Do you really need all these 3 languages? Are there going to be more in the future? Is it only a segment of these 'Noticias' that you want to show?

Answering these questions you should come up with:

  1. A way to, in the eventuality of adding another language, you won't have to alter the function;

  2. A way to increase the query performance, because fetching 7k records is different from recording the 7k x 154 languages

  3. Do you really need all 7k?

You can, for example, split that query up into several (if you really require all languages) or validate it with an advanced function within your Eager Loading statement.

Noticia::with(['langs' => function ($query) {
    //where this lang_id == 1, select it as whatever name you require
}])
.
.
.

Second is, are you sure you want those wheres exactly how they are? You're not filtering much with orWhere or Where. If you plan on narrowing down the search, I advice having a function on your where statement, just like the example shown above. If you'd like to see what the query is all about, write:

dd($data['noticias']->toSql());

Getting the images for all those news is going to be heavy, if you claim to have 70k over 7k news. You don't need to really load everything out of the start, you can use a simple ->paginate(15); to narrow it down, along side a ->skip(15);

Upvotes: 1

Related Questions