If this is going to be handled in a plugin it MUST be more abstract than com_content.article. Newsfeeds, banners, weblinks, contacts, and categories ALL have hits columns. This should NOT be an inconsistently implemented behavior. Please, things are already screwed up enough.

Actually, IIRC, these plugin events don't get run on cached views either. That's why #8887 was submitted to move hit counting out of the view in the first place. So this would be a step backwards.

if this can be considered a "not so too bad approach" i will be happy with your help to enanche/abstract to the needed degree

if i'm not wrong, i'm testing with redis cache enabled, and the hit counter is incremented when i'm in frontend com_content.artcicle context..... or at least this is what i see from backend popular articles module

The view should only be triggered if there isn't a valid cache hit. Which
honestly with Redis I can't say if it's working right or not based on
patches I added in my larger JCache PR. Otherwise the cache layer is more
busted than I realized.

On Wednesday, April 6, 2016, Nicola Galgano notifications@github.com
wrote:

if i'm not wrong, i'm testing with redis cache enabled, and the hit
counter is incremented when i'm in frontend com_content.artcicle
context..... or at list this is what i see from backend popular articles
module

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9766 (comment)

oops if i use file cache instead of redis cache then happens exactly what you say...

The solution i am using for a few years, is to increments hits inside event:

onAfterRender()

@ggppdk do have you experienced any cache issue ?

Still not a complete solution, but it is at least triggered even with some of the caching mechanisms turned on. The issue with relying on that event is if you've got the page cache set up that event doesn't get triggered.

• but our purpose is to make it work with view caching, right ? which is very common

and sites using page caching can probably live without hits counting,
with page caching many things do not run

Guess it all depends on how accurate you want that hit counter. As long as the page cache plugin isn't turned on it can work with at least conservative caching enabled globally because the component's MVC layer will get triggered and the cache gets pulled by JControllerLegacy::display() (I can't say I've ever tested the progressive cache to say what it does or doesn't trigger). I still wouldn't suggest using a plugin for the hit counter though for a single extension.

.... what about using an Ajax approach ?....

• if the view is using content plugins then we can place the onAfterRender() inside the existing content plugin joomla.php, otherwise it should be inside a system plugin

for start we can use for joomla article view, and then remove hits from controller or views of other components too and move them inside the plugin

• still i think that leaving the code inside every controller is best solution and only supporting hits counting if using "view" caching and not supporting it if using page caching

here is an example of using onAfterRender event, which i tested with all following views, (but hits counting from controller or view should be removed to avoid duplicate counting)

/*
    Tables that will not do hits inside their controller or inside their view
    a wrong entry here will not cause error, just the hit will not be done
*/
var $tblnames = array(
    'com_content'=>array(
        'article'=>'JTableContent',
        'category'=>'JTableCategory'
    ),
    'com_newsfeeds'=>array('newsfeed'=>'NewsfeedsTableNewsfeed'),
    'com_tags'=>array('tag'=>'TagsTableTag')
);

public function onAfterRender()
{
    $this->countHit();
}

public function countHit()
{
    $app = JFactory::getApplication();
    $jinput = $app->input;

    if (!$app->isSite())  return;

    $option = $jinput->getCmd('option');
    $view   = $jinput->getCmd('view');
    $task   = $jinput->getCmd('task');
    $id     = $jinput->getVar('id');

    if ( empty($id) || $task || !isset($this->tblnames[$option][$view]) ) return;

    JTable::addIncludePath(JPATH_COMPONENT_ADMINISTRATOR.'/tables');
    $tbl_name = $this->tblnames[$option][$view];

    $table = JTable::getInstance( $tbl_name, '' );

    // Check table was found, and increment hits for all given ids
    if ( $table )
    {
        $ids = is_array( $id )  ? $id : array( $id );
        JArrayHelper::toInteger($ids);

        echo "DOING hits on -- ".$tbl_name ." -- IDs: ".implode(',', $ids);
        foreach ($ids as $id) $table->hit($id);
    }
}

only benefit of using onAfterRender inside a system plugin compared to leaving the code in the controller is that it can work with Joomla cache plugin too

Still maybe not make some drastic change right now,

just patch the controller of the com_content and postpone decision of this to future

just add the method_exists() at the controller (and optionally also check layout)

• and do a more proper solution in the future

Do NOT do it as a method_exists() check. Just because the method exists does NOT mean you want to make a hit. Hell just do the layout check and commit the darn thing already.

@mbabker
yes, you are very right !,

• but it is like this already, because current code does not make any other check if the counting hit is really appropriate

so method_exists / is_callable does not add more cases, i did not suggest it as a measure of deciding the hit, it is just a sanity check for the existing code

... so i would say add both checks, but you can add only layout check , and skip the sanity check, i am not making any decisions

@alikon
can you update this pull request
and patch existing code here:
(since mbabker ?agrees? it is best to patch existing code)
https://github.com/joomla/joomla-cms/blob/0e22713228dba29f2fbd3c045a27572883e0cd5a/components/com_content/controller.php#L109-L111

with both checks or only with the layout check

        if ($vName == 'article')
        {
            // Get/Create the model
            if ($model = $this->getModel($vName))
            {
                if ( is_callable(array($model, 'hit')) && $this->input->get('layout') != 'pagebreak')
                {
                    $model->hit();
                }
            }
        }

If you're going to start adding these is_callable() or method_exists() type checks here you're going to start a trend that is going to mandate they be used everywhere since the MVC layer has no consistent interface in component level classes (seriously, look at the signatures on getItem() methods). That needs to be avoided. If you're doing is_callable() or method_exists() type checks then your code is doing far too much (which as I pointed out is happening; there is ONE display task in use for EVERY view in com_content frontend).

you are again right,
but also consider the fact that the frontend controller sometimes is given the backend model instead of the frontend model, and maybe this is not good policy ?

i don't have perfect fix to suggest without making too many changes and risk adding bugs

And again, what that means is that the frontend's ContentController::display() (not to be confused with the same method of the same named class from the admin side) is handling far too much in that single method in that it supports the processing of views and models for both the frontend AND backend components (by way of the proxy configured in the constructor). By my count this single method is processing view logic for no less than 9 core layouts spread across 9 view classes in both sides of the component. Layouts don't directly map to a routable item, so that leaves the handling of views. The ContentViewArticle class name maps to at least 4 layouts in 2 different view classes using two different model classes (ironically also of the same name) and those models have two vastly different functions. This is why the MVC layer as it is is so flawed.

Any fix is a hack. Plain and simple. The only true fix for all of this is to break B/C and rewrite the entire MVC layer.

i agree that the "display" task is handling many different cases (as you described)

• but that does not make the MVC layer flawed, just needs improving, if we had a different task then we would more easily know which model we are loading

And it can be fixed without B/C problems

• e.g. pagebreak should use a different task "pagebreak"

then for B/C purpose for "known" layouts / cases

• we can add a call to the new task from inside display task, when the layout "pagebreak is detected", until this can be removed with J4

of course you know more than me about best implementation, about whatever else needs to be revised, i am just thinking out loud

but that does not make the MVC layer flawed, just needs improving

It can't be done in a B/C way. Unless you can make MVC classes have unique names (both site and admin and when the format causes a different class to be viewed). If it were somehow aware that (pseudo-class) ContentViewHtmlPagebreak were loaded or the model classes weren't segmented based on the type of view you're loading (which is how you have a ContentModelArticle extending JModelAdmin and another extending JModelItem) things like this would be less of an issue.

now i understand which changes you mean

blame me if you want but i'm going to propose "another crap fix"

to avoid the Fatal error: Call to undefined method ContentModelArticle::hit() as reported in #9748

why not to add a void hit() function in the admin content model ?

    public function hit()
    {
        return;
    }

What about adding a $this->app->isSite() check into the plugin? (and add $app as class parameter)

see #9821