Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Feature caching improvements #150

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Feature caching improvements #150

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
353c13d
Improve caching, fix some minor issues and fix #148
JN-Jones May 22, 2015
bf8d877
Fix some more queries
JN-Jones May 22, 2015
24a5e33
Remove one more query
JN-Jones May 22, 2015
0748305
Remove one debug leftover and fix codesniffer
JN-Jones May 22, 2015
dd49ebe
Rename whereSlug to getBySlug
JN-Jones May 26, 2015
c4aaea2
Merge branch 'fix-composer-update' into feature-caching_improvements
JN-Jones Aug 25, 2015
b3bc533
Forgot to rerun composer
JN-Jones Aug 25, 2015
1fe2074
Merge branch 'master' into feature-caching_improvements
JN-Jones Oct 19, 2015
b0091d0
Minor improvement for forum caching
JN-Jones Oct 20, 2015
6df6d37
Add AbstractCachingModel to moderation log classes
JN-Jones Oct 20, 2015
c7cffe5
Run composer update
JN-Jones Oct 28, 2015
fa4a78f
Run composer update once again
JN-Jones Nov 4, 2015
75f0378
Merge branch 'master' into feature-caching_improvements
JN-Jones Jan 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 57 additions & 0 deletions app/Database/Collections/TreeCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php
/**
* Tree collection class.
*
* @author MyBB Group
* @version 2.0.0
* @package mybb/core
* @license http://www.mybb.com/licenses/bsd3 BSD-3
*/

namespace MyBB\Core\Database\Collections;

use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;

class TreeCollection extends Collection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a collection of Tree objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a collection of objects which can be displayed as tree.

{
/**
* Fill `parent` and `children` relationships for every node in collection.
*
* This will overwrite any previously set relations.
*
* @return $this
*/
public function linkNodes()
{
if ($this->isEmpty()) {
return $this;
}
$groupedChildren = $this->groupBy('parent_id');
/** @var Model $node */
foreach ($this->items as $node) {
if (!isset($node->parent)) {
$node->setRelation('parent', null);
}
$children = $groupedChildren->get($node->getKey(), []);
/** @var Model $child */
foreach ($children as $child) {
$child->setRelation('parent', $node);
}
$node->setRelation('children', Collection::make($children));
}

return $this;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very clear on what this class is for and what this method does? Could you elaborate on the doc block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's copied from the nestedset library. Including the doc block. It seems to update the parent/children relations and that's mentioned in the doc block too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, when is toTree() and linkNodes() being called? It's a bit weird that there's this method that does stuff to objects in a collection and then it returns itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toTree is called whenever we display forums in Twig: for forum.children.toTree. linkNodes is called within toTree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does linkNodes() alter the database?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it only makes sure the relations are correct and especially makes sure that the children relation is a collection which itself is a tree too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so TreeCollection has two responsibilities, representing a collection of objects that can be represented as a tree and then also linking those objects together and ensuring the integrity of the relationships. I would break the latter out into a separate class.

$forumCollection = new ForumCollection([new Forum()]);
$linker = new NodeLinker();
$tree = $linker->link($forumCollection); // $tree implements TreeInterface?

Or

$forumCollection = new ForumCollection([new Forum()]);
$treeFactory = new TreeFactory();
$tree = $treeFactory->buildFromCollection($forumCollection);

Something like that off the top of my head. Essentially, one thing that represents a collection of objects to be linked, one thing that does the linking and one thing that represents the linked objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said: It's simply the same as the nestedset library does. And the reason to remove that was to simplify things. And calling for forum.children.toTree is a lot simpler than what you're suggesting. Also the TreeCollection has exactly one responsibility: Return a given collection as tree representation. That it rechecks/links nodes is simply needed to do so (it doesn't make sense to update the parent node without the child nodes).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok if we say that TreeCollection has one responsibility, I disagree but fine. It's API still doesn't make a lot of sense to me. Take this example, where I have Link objects which can be put together to represent a chain:

class Link
{
    protected $previous;
    protected $next;

    public function linkPrevious(Link $previous)
    {
        $this->previous = $previous;
    }

    public function linkNext(Link $next)
    {
        $this->next = $next;
    }
}

class ChainCollection
{
    protected $links;

    public function __construct(array $links)
    {
        $this->links = $links;
    }

    public function linkLinks()
    {
        $previous = null;
        foreach ($this->links as $link) {
            if (! $previous) {
                $previous = $link;
                continue;
            }
            $previous->linkNext($link);
            $link->linkPrevious($previous);
            $previous = $link;
        }
    }

    public function toChain()
    {
        $this->linkLinks();
        return $this;
    }
}

$chain = new ChainCollection([new Link(), new Link()]); // not a chain
$chain = $chain->toChain(); // now it's a chain

We've got a bunch of Link objects and we put them into an object called ChainCollection but they are not a chain yet because they are not linked, so it's a useless object until all of the links are linked by calling toChain() which calls linkLinks() which does the linking.

We're never going to construct ChainCollection without calling toChain() in this instance.

We can clean up the API so that this makes more sense and more accurately represents what we're trying to model:

class Link
{
    protected $previous;
    protected $next;

    public function linkPrevious(Link $previous)
    {
        $this->previous = $previous;
    }

    public function linkNext(Link $next)
    {
        $this->next = $next;
    }
}

class Chain
{
    protected $links;

    public function __construct(array $links)
    {
        $this->links = $links;
        $previous = null;
        foreach ($this->links as $link) {
            if (! $previous) {
                $previous = $link;
                continue;
            }
            $previous->linkNext($link);
            $link->linkPrevious($previous);
            $previous = $link;
        }
    }
}

$chain = new Chain([new Link(), new Link()]); // a valid chain

This makes more sense, I'm going to construct a chain and once constructed I expect that object to be a valid representation of a chain.

I think there's things in this example that apply to TreeCollection.


/**
* Build tree from node list. Each item will have set children relation.
*
* @return Collection
*/
public function toTree()
{
$this->linkNodes();
return $this;
}
}
10 changes: 5 additions & 5 deletions app/Database/Models/AbstractCachingModel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function save(array $options = array())
$saved = parent::save($options);

if ($saved) {
static::$models[$this->getKey()] = $this;
static::$models[get_class($this)][$this->getKey()] = $this;
}

return $saved;
Expand All @@ -37,7 +37,7 @@ public function save(array $options = array())
public function delete()
{
parent::delete();
unset(static::$models[$this->getKey()]);
unset(static::$models[get_class($this)][$this->getKey()]);
}

/**
Expand All @@ -49,10 +49,10 @@ public static function find($id, $columns = array('*'))
return parent::find($id, $columns);
}

if (!isset(static::$models[$id])) {
static::$models[$id] = parent::find($id);
if (!isset(static::$models[get_called_class()][$id])) {
static::$models[get_called_class()][$id] = parent::find($id);
}

return static::$models[$id];
return static::$models[get_called_class()][$id];
}
}
2 changes: 1 addition & 1 deletion app/Database/Models/Conversation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @property ConversationMessage lastMessage
* @property Collection participants
*/
class Conversation extends Model implements HasPresenter
class Conversation extends AbstractCachingModel implements HasPresenter
{
/**
* The database table used by the model.
Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/ConversationMessage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* @property Conversation conversation
* @property User author
*/
class ConversationMessage extends Model implements HasPresenter
class ConversationMessage extends AbstractCachingModel implements HasPresenter
{
/**
* The database table used by the model.
Expand Down
50 changes: 41 additions & 9 deletions app/Database/Models/Forum.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,13 @@

use MyBB\Core\Permissions\Interfaces\InheritPermissionInterface;
use MyBB\Core\Permissions\Traits\InheritPermissionableTrait;
use Kalnoy\Nestedset\Node;
use McCool\LaravelAutoPresenter\HasPresenter;
use MyBB\Core\Database\Collections\TreeCollection;

class Forum extends Node implements HasPresenter, InheritPermissionInterface
class Forum extends AbstractCachingModel implements HasPresenter, InheritPermissionInterface
{
use InheritPermissionableTrait;

/**
* Nested set column IDs.
*/
const LFT = 'left_id';
const RGT = 'right_id';
const PARENT_ID = 'parent_id';

// @codingStandardsIgnoreStart
/**
* Indicates if the model should be timestamped.
Expand Down Expand Up @@ -65,6 +58,18 @@ public function getPresenterClass()
return 'MyBB\Core\Presenters\Forum';
}

/**
* @return InheritPermissionableTrait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the return type of this method?

*/
public function getParent()
{
if ($this->parent_id === null) {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does find() not behave like this if it's passed a null value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to run a query though. At least it removed two queries for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, makes sense.


return $this->find($this->parent_id);
}

/**
* A forum contains many threads.
*
Expand Down Expand Up @@ -104,4 +109,31 @@ public function lastPostAuthor()
{
return $this->hasOne('MyBB\\Core\\Database\\Models\\User', 'id', 'last_post_user_id');
}

/**
* Relation to the parent.
*
* @return \Illuminate\Database\Eloquent\Relations\BelongsTo
*/
public function parent()
{
return $this->belongsTo(get_class($this), 'parent_id');
}
/**
* Relation to children.
*
* @return \Illuminate\Database\Eloquent\Relations\HasMany
*/
public function children()
{
return $this->hasMany(get_class($this), 'parent_id');
}

/**
* {@inheritdoc}
*/
public function newCollection(array $models = array())
{
return new TreeCollection($models);
}
}
2 changes: 1 addition & 1 deletion app/Database/Models/Permission.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

use Illuminate\Database\Eloquent\Model;

class Permission extends Model
class Permission extends AbstractCachingModel
{

/**
Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/Poll.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use Illuminate\Database\Eloquent\Model;
use McCool\LaravelAutoPresenter\HasPresenter;

class Poll extends Model implements HasPresenter
class Poll extends AbstractCachingModel implements HasPresenter
{
// @codingStandardsIgnoreStart

Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/PollVote.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

use Illuminate\Database\Eloquent\Model;

class PollVote extends Model
class PollVote extends AbstractCachingModel
{
// @codingStandardsIgnoreStart

Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/Post.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
use McCool\LaravelAutoPresenter\HasPresenter;
use MyBB\Core\Likes\Traits\LikeableTrait;

class Post extends Model implements HasPresenter
class Post extends AbstractCachingModel implements HasPresenter
{
use SoftDeletes;
use LikeableTrait;
Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/ProfileField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* @property string validation_rules
* @property string name
*/
class ProfileField extends Model implements HasPresenter
class ProfileField extends AbstractCachingModel implements HasPresenter
{
/**
* @var string
Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/ProfileFieldGroup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
/**
* @property int id
*/
class ProfileFieldGroup extends Model implements HasPresenter
class ProfileFieldGroup extends AbstractCachingModel implements HasPresenter
{
const ABOUT_YOU = 'about-you';
const CONTACT_DETAILS = 'contact-details';
Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/ProfileFieldOption.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* @property string name
* @property string value
*/
class ProfileFieldOption extends Model
class ProfileFieldOption extends AbstractCachingModel
{
/**
* @var string
Expand Down
59 changes: 56 additions & 3 deletions app/Database/Models/Role.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@

namespace MyBB\Core\Database\Models;

use Illuminate\Database\Eloquent\Model;

class Role extends Model
class Role extends AbstractCachingModel
{
/**
* @var array
*/
protected static $slugCache;

/**
* The database table used by the model.
Expand All @@ -35,4 +37,55 @@ public function permissions()
{
return $this->belongsToMany('MyBB\Core\Database\Models\Permission');
}

/**
* @param string $slug
*
* @return Role
*/
public static function whereSlug($slug)
{
if (!isset(static::$slugCache[$slug])) {
static::$slugCache[$slug] = static::where('role_slug', '=', $slug)->first();
}

return static::$slugCache[$slug];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't where*() methods supposed to return a query builder object or similar? If this function is just returning a runtime cached object, then getBySlug() is probably a better function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching a QueryBuilder object doesn't make sense as it still would run a query. Fine with getBySlug

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, getBySlug() sounds good.


/**
* {@inheritdoc}
*/
public function save(array $options = array())
{
$saved = parent::save($options);

if ($saved) {
static::$slugCache[$this->role_slug] = $this;
}

return $saved;
}

/**
* {@inheritdoc}
*/
public function delete()
{
parent::delete();
unset(static::$slugCache[$this->role_slug]);
}

/**
* {@inheritdoc}
*/
public static function find($id, $columns = array('*'))
{
$model = parent::find($id, $columns);

if ($columns == array('*')) {
static::$slugCache[$model->role_slug] = $model;
}

return $model;
}
}
2 changes: 1 addition & 1 deletion app/Database/Models/Search.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

use Illuminate\Database\Eloquent\Model;

class Search extends Model
class Search extends AbstractCachingModel
{
// @codingStandardsIgnoreStart

Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/Topic.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
use Illuminate\Database\Eloquent\SoftDeletes;
use McCool\LaravelAutoPresenter\HasPresenter;

class Topic extends Model implements HasPresenter
class Topic extends AbstractCachingModel implements HasPresenter
{
use SoftDeletes;

Expand Down
4 changes: 2 additions & 2 deletions app/Database/Models/User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
/**
* @property string id
*/
class User extends Model implements AuthenticatableContract, CanResetPasswordContract, HasPresenter, PermissionInterface
class User extends AbstractCachingModel implements AuthenticatableContract, CanResetPasswordContract, HasPresenter, PermissionInterface
{
use Authenticatable;
use CanResetPassword;
Expand Down Expand Up @@ -109,7 +109,7 @@ public function displayRole()
if ($this->displayRole == null) {
// Do we have a guest?
if ($this->id <= 0) {
$this->displayRole = Role::where('role_slug', 'guest')->first();
$this->displayRole = Role::whereSlug('guest');
} else {
$this->displayRole = $this->roles->whereLoose('pivot.is_display', true)->first();
}
Expand Down
2 changes: 1 addition & 1 deletion app/Database/Models/UserProfileField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
/**
* @property string value
*/
class UserProfileField extends Model
class UserProfileField extends AbstractCachingModel
{
/**
* @var string
Expand Down