Lack of interest AbstractCriteria is not abstract

[CMTV]

Hey!

Imagine we are creating an addon for removing all posts that match selected criteria. A list of available criteria:

• Post has at least X likes
• Post author has an X username
• Post was edited at least X times
• Post was edited no more than X times
• Post was published before X
• Post was published after X
  ...

Obviously, for such criteria we need a new criteria type: Post criteria, which means we are to inherit AbstractCriteria class.

And everything works great until the moment you are trying to use isMatched(...) method...

...Because it requires you to pass a User entity as an argument... WTF?

public function isMatched(\XF\Entity\User $user)
{

I think a User entity should be required in User criteria class, not in AbstractCriteria because it simply kills the whole idea of different criteria types...

In the example above, isMatched should take a Post entity.

public function isMatched(\XF\Entity\Post $post)
{

My suggestion: isMatched should take an argument of type Entity or take nothing at all (leaving this job of type clarifying on AbstractCriteria ancestors).

Why I think it is an issue? Because, in general, AbstractCriteria is written on an abstract level. It only provides methods to work wit criteria no matter what data they contain.

 

[CMTV]

Please, don't add "As designed" prefix to this the thread

 

[Chris D]

Well, we might.

Frankly the use case for criteria based stuff has always been whether a user matches any given criteria. There are sort of extensions of that in terms of page criteria (what page the user is viewing) and date criteria (still mostly relative to the user itself) so matching criteria against other content arbitrarily has never been a desired target.

So in that sense it is indeed as designed.

I don’t have the code in front of me right now so I will at least look it over but I’m almost certain that we wouldn’t be looking to make any changes to that method specifically.

Maybe you can still extend the system to do what you want though and certainly that’s what you should be looking to do in the short to medium term.

 

[DragonByte Tech]

While it's not ideal, it's very easy to work around, this is my Thread match criteria class which you can easily adapt to your needs.

<?php

namespace Your\Namespace\Criteria;

use XF\Criteria\AbstractCriteria;

class Thread extends AbstractCriteria
{
    /**
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    public function isMatchedThread(\XF\Entity\Thread $thread)
    {
        if (!$this->criteria)
        {
            return $this->matchOnEmpty;
        }
        
        foreach ($this->criteria AS $criterion)
        {
            $rule = $criterion['rule'];
            $data = $criterion['data'];
            
            $specialResult = $this->isSpecialMatchedThread($rule, $data, $thread);
            if ($specialResult === false)
            {
                return false;
            }
            else if ($specialResult === true)
            {
                continue;
            }
            
            $method = '_matchThread' . \XF\Util\Php::camelCase($rule);
            if (method_exists($this, $method))
            {
                $result = $this->$method($data, $thread);
                if (!$result)
                {
                    return false;
                }
            }
            else
            {
                if (!$this->isUnknownMatchedThread($rule, $data, $thread))
                {
                    return false;
                }
            }
        }
        
        return true;
    }
    
    /**
     * @param $rule
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool|null
     */
    protected function isSpecialMatchedThread($rule, array $data, \XF\Entity\Thread $thread)
    {
        // custom user fields
        if (preg_match('/^thread_field_(.+)$/', $rule, $matches))
        {
            /** @var \XF\CustomField\Set|null $cFS */
            $cFS = $thread->thread_id ? $thread->custom_fields : null;
            
            $fieldId = $matches[1];
            
            if (!$cFS || !isset($cFS->{$fieldId}))
            {
                return false;
            }
            
            $value = $cFS->{$fieldId};
            
            // text fields - check that data exists within the text value
            if (isset($data['text']))
            {
                if (stripos($value, $data['text']) === false)
                {
                    return false;
                }
            }
            // choice fields - check that data is in the choice array
            else if (isset($data['choices']))
            {
                // multi-choice
                if (is_array($value))
                {
                    if (!array_intersect($value, $data['choices']))
                    {
                        return false;
                    }
                }
                // single choice
                else
                {
                    if (!in_array($value, $data['choices']))
                    {
                        return false;
                    }
                }
            }
            
            return true;
        }
        
        return null;
    }
    
    /**
     * @param $rule
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function isUnknownMatchedThread($rule, array $data, \XF\Entity\Thread $thread)
    {
        $eventReturnValue = false;
        $this->app->fire('your_namespace_criteria_thread', [$rule, $data, $thread, &$eventReturnValue]);
        
        return $eventReturnValue;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadForum(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->node_id && in_array($thread->node_id, $data['forum_ids']));
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadPrefix(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->prefix_id && in_array($thread->prefix_id, $data['prefix_ids']));
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadReplyCount(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->reply_count && $thread->reply_count >= $data['messages']);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadReplyCountMaximum(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->reply_count <= $data['messages']);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadViewCount(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->view_count && $thread->view_count >= $data['amount']);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadViewCountMaximum(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->view_count <= $data['amount']);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadDiscussionState(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->discussion_state && $thread->discussion_state == $data['state']);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadIsOpen(array $data, \XF\Entity\Thread $thread)
    {
        return (bool)$thread->discussion_open;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadNoOpen(array $data, \XF\Entity\Thread $thread)
    {
        return $thread->discussion_open ? false : true;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadIsSticky(array $data, \XF\Entity\Thread $thread)
    {
        return (bool)$thread->sticky;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadNoSticky(array $data, \XF\Entity\Thread $thread)
    {
        return $thread->sticky ? false : true;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterIsGuest(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->user_id == 0);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterIsMember(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->user_id > 0);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterIsModerator(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->User)
        {
            return false;
        }
        
        return (bool)$thread->User->is_moderator;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterIsAdmin(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->User)
        {
            return false;
        }
        
        return (bool)$thread->User->is_admin;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterTime(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->post_date)
        {
            return false;
        }
        $threadAge = floor((time() - $thread->post_date) / 86400);
        if ($threadAge < $data['days'])
        {
            return false;
        }
        
        return true;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterLikeCount(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->first_post_likes && $thread->first_post_likes >= $data['likes']);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterUsername(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->username)
        {
            return false;
        }
        
        $names = preg_split('/\s*,\s*/', utf8_strtolower($data['names']), -1, PREG_SPLIT_NO_EMPTY);
        return in_array(utf8_strtolower($thread->username), $names);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadStarterUsernameSearch(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->username)
        {
            return false;
        }
        
        if ($this->findNeedle($data['needles'], $thread->username) === false)
        {
            return false;
        }
        else
        {
            return true;
        }
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyIsGuest(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->last_post_user_id == 0);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyIsMember(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->last_post_user_id > 0);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyIsStarter(array $data, \XF\Entity\Thread $thread)
    {
        return ($thread->last_post_user_id == $thread->user_id);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyIsModerator(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->LastPoster)
        {
            return false;
        }
        
        return (bool)$thread->LastPoster->is_moderator;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyIsAdmin(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->LastPoster)
        {
            return false;
        }
        
        return (bool)$thread->LastPoster->is_admin;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyTime(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->last_post_date)
        {
            return false;
        }
        $lastReplyAge = floor((time() - $thread->last_post_date) / 86400);
        if ($lastReplyAge < $data['days'])
        {
            return false;
        }
        
        return true;
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyUsername(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->last_post_username)
        {
            return false;
        }
        
        $names = preg_split('/\s*,\s*/', utf8_strtolower($data['names']), -1, PREG_SPLIT_NO_EMPTY);
        return in_array(utf8_strtolower($thread->last_post_username), $names);
    }
    
    /**
     * @param array $data
     * @param \XF\Entity\Thread $thread
     *
     * @return bool
     */
    protected function _matchThreadLastReplyUsernameSearch(array $data, \XF\Entity\Thread $thread)
    {
        if (!$thread->last_post_username)
        {
            return false;
        }
        
        if ($this->findNeedle($data['needles'], $thread->last_post_username) === false)
        {
            return false;
        }
        else
        {
            return true;
        }
    }
    
    
    /**
     * @return array
     */
    public function getExtraTemplateData()
    {
        $templateData = parent::getExtraTemplateData();
        
        /** @var \XF\Repository\Node $nodeRepo */
        $nodeRepo = $this->app->repository('XF:Node');
        $nodes = $nodeRepo->createNodeTree($nodeRepo->getFullNodeList());
        
        /** @var \XF\Repository\ThreadPrefix $prefixRepo */
        $prefixRepo = \XF::repository('XF:ThreadPrefix');
        $availablePrefixes = $prefixRepo->findPrefixesForList()->fetch()->pluckNamed('title', 'prefix_id');
        $prefixListData = $prefixRepo->getPrefixListData();
        
        $templateData = array_merge($templateData, [
            'nodeTree' => $nodes,
            
            'availablePrefixes' => $availablePrefixes,
            
            'prefixGroups' => $prefixListData['prefixGroups'],
            'prefixesGrouped' => $prefixListData['prefixesGrouped']
        ]);
        
        return $templateData;
    }
}

Then you can call it like so:

/** @var \Your\Namespace\Criteria\Thread $threadCriteria */
$threadCriteria = $this->app()->criteria('Your\Namespace:Thread', $criteria);
$threadCriteria->setMatchOnEmpty(false);
if ($threadCriteria->isMatchedThread($thread))
{
    // Cool stuff happens here
}

Remember to change the Your\Namespace and your_namespace to relevant values.

You could argue that Thread and Post criteria should be built into the core, and I'd agree, but for now it's not impossible to create your own criteria class.


Fillip

 

[CMTV]

@Chris D, I thought that the main idea of Criteria system can be expressed like this:

"When XenForo needs to test something (user/page/post/thread...) against some user defined conditions (criteria), it uses the Criteria system."

From that position, the right one (in my opinion), the AbstractCriteria should be "context free" and only propose methods to work with criterion "rule-data" model. isMatched can be separated into two methods: isMatchedEmpty() and isMatched($obj). In general - need to think over the best way.

AbstractCriteria class, for example, has an ancestor called AbstractUserCriteria which has 2 ancestors: User and Page classes.

I mean, the "problem" is only with this particular method + isSpecialMatched and isUnknownMatched which are derivative. No need to rewrite the whole class. At least on the first glance.

---

Rephrasing the problem — I don't think this is cool to be forced to bypass isMatched restriction (as @DragonByte Tech proposed) in order to use AbstractCriteria handy functionality because this method is limited to Users only, which is not obvious and breaks the expectations from something being prefixed Abstract especially when you already have User ancestor class.

---

I asked this question because I am preparing (almost done, in fact ) a pull request for xf2docs with a big article about criteria system: what it is, how to use, examples. One third of this article is about creating a custom criteria type: an example I described in the first post. So I guess I need to remove this part, for now.

 

[Chris D]

Now I have looked into it further, I still standby the current behaviour. Architecturally, we may have been able to do it slightly differently, but I don't think the approach was entirely invalid.

Whichever structure was decided upon doesn't prevent the class being extended in the way that @DragonByte Tech suggests and this would be the recommended approach right now. Certainly for 2.0 at least. Bear in mind that this kind of structural change is not something we can likely easily change in a third point release as it would almost certain represent a breaking change. We're also fairly close to the end of 2.1 so it may not change there either.

In that case, it just means "making do" for the time being. We'll comment further once we've had a chance to discuss it internally.

 

[CMTV]

I understand why it can't be changed in 2.0. Really hope this will be "fixed?" in the 2.1 because, well, it will require addon makers to review their code for possible incompatibilities anyways.