2
\$\begingroup\$

I have made a Laravel 8 blogging application that supports themes.

I had made a custom 404 template for every theme, the path to it being 'resources\views\themes\' . $theme_directory . '\templates\404.blade.php', where $theme_directory is the name of the directory of the theme.

The active theme is set in the settings table, and the parent controller reads it from there:

class FrontendController extends Controller
{
  protected $data;
  protected $site_settings;
  protected $theme_directory;
  protected $site_name;
  protected $tagline;
  protected $owner_name;
  protected $owner_email;
  protected $twitter;
  protected $facebook;
  protected $instagram;
  protected $is_cookieconsent;
  protected $is_infinitescroll;
  protected $pages;
  protected $articles;
  protected $article_categories;
  protected $article_tags;
  protected $authors;

  public function __construct()
  {
    $this->site_settings = Settings::first();
    $this->theme_directory = $this->site_settings['theme_directory'] ?? null;
    $this->site_name = $this->site_settings['site_name'] ?? null;
    $this->tagline = $this->site_settings['tagline'] ?? null;
    $this->owner_name = $this->site_settings['owner_name'] ?? null;
    $this->owner_email = $this->site_settings['owner_email'] ?? null;
    $this->twitter = $this->site_settings['twitter'] ?? null;
    $this->facebook = $this->site_settings['facebook'] ?? null;
    $this->instagram = $this->site_settings['instagram'] ?? null;
    $this->is_cookieconsent = $this->site_settings['is_cookieconsent'] ?? null;
    $this->is_infinitescroll = $this->site_settings['is_infinitescroll'] ?? null;

    // Most recent articles
    $this->articles = Article::visible()->limit(5)->get();

    // Article categories. Get only categories with articles  
    $this->article_categories = ArticleCategory::has('articles')->get();

    // Pages
    $this->pages = Page::all();

    // Article tags
    $this->article_tags = Tag::all();

    // Authors
    $this->authors = User::withCount('articles')
      ->having('articles_count', '>', 0)
      ->orderByDesc('articles_count')
      ->get();

    $this->data = [
      'theme_directory' => $this->theme_directory,
      'site_name' => $this->site_name,
      'tagline' => $this->tagline,
      'owner_name' => $this->owner_name,
      'owner_email' => $this->owner_email,
      'twitter' => $this->twitter,
      'facebook' => $this->facebook,
      'instagram' => $this->instagram,
      'is_cookieconsent' => $this->is_cookieconsent,
      'is_infinitescroll' => $this->is_infinitescroll,
      'pages' => $this->pages,
      'articles' => $this->articles,
      'categories' => $this->article_categories,
      'tags' => $this->article_tags,
      'authors' => $this->authors,
    ];
  }
}

But Laravel displayed its default 404 view.

I solved this by adding a render() method in app\Exceptions\Handler.php, wich made look like so:

namespace App\Exceptions;
use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
use Throwable;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use App\Models\Settings;
use App\Models\Page;
use App\Models\ArticleCategory;

class Handler extends ExceptionHandler
{
  /**
   * A list of the exception types that are not reported.
   *
   * @var array<int, class-string<Throwable>>
   */
  protected $dontReport = [
    //
  ];

  /**
   * A list of the inputs that are never flashed for validation exceptions.
   *
   * @var array<int, string>
   */
  protected $dontFlash = [
    'current_password',
    'password',
    'password_confirmation',
  ];

  /**
   * Register the exception handling callbacks for the application.
   *
   * @return void
   */
  public function register()
  {
    $this->reportable(function (Throwable $e) {
      //
    });
  }

  public function render($request, Throwable $exception)
  {
    if ($exception instanceof ModelNotFoundException || $exception instanceof NotFoundHttpException) {

      // Get the active theme
      $settings = Settings::first();

      // Pages and categories
      $pages = Page::all();
      $article_categories = ArticleCategory::has('articles')->get();

      // Check if that view exists
      if (view()->exists('themes/' . $settings->theme_directory . '/templates/404')) {
         $theme_404_view = 'themes/' . $settings->theme_directory . '/templates/404';

        return response()->view($theme_404_view, [
          'theme_directory' => $settings->theme_directory,
          'site_name' => $settings->site_name,
          'tagline' => $settings->tagline,
          'owner_name' => $settings->owner_name,
          'owner_email' => $settings->owner_email,
          'twitter' => $settings->twitter,
          'facebook' => $settings->facebook,
          'instagram' => $settings->instagram,
          'is_cookieconsent' => $settings->is_cookieconsent,
          'is_infinitescroll' => $settings->is_infinitescroll,
          'title' => "404",
          'subtitle' => "Article not found",
          'message'=> "Nothing to see here!",
          'pages' => $pages,
          'categories' => $article_categories
        ], 404);
      }
    }

    return parent::render($request, $exception);
  }
}

In app\Http\Controllers\ArticlesController.php I have:

class ArticlesController extends FrontendController
{
 protected $per_page = 12;
 protected $comments_per_page = 10;
 protected $comments_orderby_direction = 'desc';

 public function index(Request $request)
 {
    if (Settings::count() == 0) {
        return redirect()->route('dashboard');
    }

    $qry = $request->input('search');
    $articlesQuery = Article::visible();

    if (!empty($qry)) {
        $articlesQuery->where(function ($q) use ($qry) {
            $q->where('title', 'like', '%' . $qry . '%')
              ->orWhere('short_description', 'like', '%' . $qry . '%')
              ->orWhere('content', 'like', '%' . $qry . '%');
        });
        $article_count = $articlesQuery->count();
    }

    $articles = $articlesQuery->paginate($this->per_page);

    $featured_articles = Article::visible()
        ->where('featured', 1)
        ->get();

    return view(
        'themes/' . $this->theme_directory . '/templates/index',
        array_merge($this->data, [
            'search_query'      => $qry,
            'articles'          => $articles,
            'featured_articles' => $featured_articles,
            'article_count'     => $article_count ?? null
        ])
    );
}

public function category($category_id)
{
    $category = ArticleCategory::firstWhere('id', $category_id);

    if ($category) {
        $articles = Article::visible()
            ->where('category_id', $category_id)
            ->paginate($this->per_page);

        return view(
            'themes/' . $this->theme_directory . '/templates/index',
            array_merge($this->data, [
                'category' => $category,
                'articles' => $articles
            ])
        );
    }

    return redirect('/404');
}

public function show($slug)
{
    $article = Article::visible()->where('slug', $slug)->firstOrFail();

    $sessionKey = 'article_viewed_' . $article->id;
    $lastViewedAt = session($sessionKey);
    if (!$lastViewedAt || Carbon::createFromTimestamp($lastViewedAt)->diffInMinutes(now()) >= 60) {
        $article->increment('views');
        session()->put($sessionKey, now()->timestamp);
    }

    $old_article = Article::visible()
        ->where(function ($q) use ($article) {
            $q->where('published_at', '<', $article->published_at)
              ->orWhere(function ($q2) use ($article) {
                  $q2->where('published_at', $article->published_at)
                     ->where('id', '<', $article->id);
              });
        })
        ->orderBy('published_at', 'desc')
        ->orderBy('id', 'desc')
        ->first();

    $new_article = Article::visible()
        ->where(function ($q) use ($article) {
            $q->where('published_at', '>', $article->published_at)
              ->orWhere(function ($q2) use ($article) {
                  $q2->where('published_at', $article->published_at)
                     ->where('id', '>', $article->id);
              });
        })
        ->orderBy('published_at', 'asc')
        ->orderBy('id', 'asc')
        ->first();

    if ($this->is_infinitescroll) {
        $comments = $this->get_commentQuery($article->id, $this->comments_per_page, 0)
            ->whereNull('parent_id')
            ->with(['user', 'replies.user'])
            ->get();

        $comments_count = $this->get_commentQuery($article->id)->count();
    } else {
        $comments = $this->get_commentQuery($article->id)
            ->whereNull('parent_id')
            ->with(['user', 'replies.user'])
            ->get();

        $comments_count = $this->get_commentQuery($article->id)->count();
    }

    return view(
        'themes/' . $this->theme_directory . '/templates/single',
        array_merge($this->data, [
            'categories'        => $this->article_categories,
            'article'           => $article,
            'old_article'       => $old_article,
            'new_article'       => $new_article,
            'comments'          => $comments,
            'comments_count'    => $comments_count,
            'comments_per_page' => $this->comments_per_page,
            'tagline'           => $article->title,
            'is_infinitescroll' => $this->is_infinitescroll
        ])
    );
  }
}

The 404 controller looks like this, and it sets the $title, $subtitle, and $message variables for the theme's 404 view, for everything but a not-found article.

class PageNotFoundController extends FrontendController 
{
  public function notfound()
  {
    return view(
      'themes/' . $this->theme_directory . '/templates/404',
      array_merge($this->data, [
        'title' => "404",
        'subtitle' => "Page not found :(",
        'message' => "Nothing to see here!"
      ])
    );
  }
}

If an article is not found, that is handled by Handler.php.

The problem is an obvious repetition of code in app\Http\Controllers\FrontendController.php and app\Exceptions\Handler.php.

Question

  1. How can I avoid the code repetition mentioned above?
  2. Are there any security vulnerabilities in my code?
  3. Is there any room for optimization?
\$\endgroup\$
0

1 Answer 1

Reset to default
2
+50
\$\begingroup\$

I didn't catch any vulnerability, but there are a couple of suggestions you might find useful.

The biggest concern I see in your code is that your controllers methods have too much responsibilities. I'd delegate to other classes in your app namespace, you can have things like App\Services namespaced files and create a class used to handle theming. In fact there's no strict rule around how you could name that, if you think you would require more themes-related stuff you might as well use App\Theme namespace and have a class like ThemeRenderer with methods dedicated to handle your themed responses.

If you see that your code is repeating, consider a good practice extracting its logic to an external class that you then import where you need it. SOLID principles are your friends here, S stands for Single responsibility and is the best thing I ever learned.

About optimizing, there are indeed a few things I'd do differently. Consider using more model query scopes. As example you could make the following:

$new_article = Article::visible()
    ->where(function ($q) use ($article) {
        $q->where('published_at', '>', $article->published_at)
          ->orWhere(function ($q2) use ($article) {
              $q2->where('published_at', $article->published_at)
                 ->where('id', '>', $article->id);
          });
    })
    ->orderBy('published_at', 'asc')
    ->orderBy('id', 'asc')
    ->first();

Become something like this in the Article model class

public function scopeNextVisible($query, Article $article)
{
    return $query->visible()
        ->where(function ($q) use ($article) {
            $q->where('published_at', '>', $article->published_at)
              ->orWhere(function ($q2) use ($article) {
                  $q2->where('published_at', $article->published_at)
                     ->where('id', '>', $article->id);
              });
        })
        ->orderBy('published_at', 'asc')
        ->orderBy('id', 'asc');
}

And you would call it like

$new_article = Article::nextVisible($article)->first();

The same applies for other things like search results and featured articles

public function scopeSearch($query, ?string $term)
{
    if (empty($term)) {
        return $query;
    }

    return $query->where(function ($q) use ($term) {
        $q->where('title', 'like', "%{$term}%")
          ->orWhere('short_description', 'like', "%{$term}%")
          ->orWhere('content', 'like', "%{$term}%");
    });
}

public function scopeFeatured($query)
{
    return $query->visible()->where('featured', 1);
}

And, at last, I would also consider optimizing these queries by adding a select with only what you actually use in the page (as example, your search results or the next or previous articles may just require the title, the id and the publish date).

I can keep an eye on your PR if you'd like! 🙌🏻

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.