Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Page Break error after upgrade Joomla 3.5.1 #9748

Closed
hienduchuynh opened this issue Apr 6, 2016 · 30 comments

Comments

@hienduchuynh
Copy link

commented Apr 6, 2016

Steps to reproduce the issue

Upgrade Joomla 3.5.0 to Joomla 3.5.1
Login Admin account in Frontend
Edit a article
Click Page Break button

Actual result

Fatal error: Call to undefined method ContentModelArticle::hit() in D:\localhost\www\test\Joomla_3.5.1-Stable-Full_Package\components\com_content\controller.php on line 109

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

I confirm the issue. This is a regression.

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

When layout==='pagebreak'

  • then the base_path is the backend to load the backend model
    which does not have the hits() method
    // Article frontpage Editor pagebreak proxying:
    if ($this->input->get('view') === 'article' && $this->input->get('layout') === 'pagebreak')
    {
    $config['base_path'] = JPATH_COMPONENT_ADMINISTRATOR;
    }
// Article frontpage Editor pagebreak proxying:
if ($this->input->get('view') === 'article' && $this->input->get('layout') === 'pagebreak')
{
    $config['base_path'] = JPATH_COMPONENT_ADMINISTRATOR;
}

but the problem is not loading the backend model,
neither the recent change should be reverted :

  • as it fixed hits not being counted when page is page is not cacheable
    0e22713

instead add an extra check if

  • method hits() exists and (also ?) if article model is the frontend model

$model->hit();
}
}

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@alikon
So i suggest using:

    // Get/Create the model
    if ( $model = $this->getModel($vName) )
    {
        if (method_exists($model, 'hit'))
            $model->hit();
        }
    }

[EDIT]
i have corrected function name 'hits' which should be 'hit'

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Another solution I tested and works fine is:

        if ($vName == 'article')
        {
            // Get/Create the model
            if ($model = $this->getModel($vName) && JFactory::getApplication()->input->get('layout') !== 'pagebreak')
            {
                $model->hit();
            }
        }
@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Please, @alikon , test my solution and I will make PR.

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Simpler:

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

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Making PR now.

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@infograf768
no better avoid it,
because it will break at any time that controler or other code decides to use backend model

the check if the model has hits() method must be there

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

I let you do then. 😃

@alikon

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

Sorry folks unable to help till evening...
On 6 Apr 2016 11:16 am, "infograf768" notifications@github.com wrote:

I let you do then. [image: 😃]


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9748 (comment)

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@infograf768
i cannot make anything till late evening
can you please make a PR for this: (so that it is done faster)

    // Get/Create the model
    if ( $model = $this->getModel($vName) )
    {
        if (method_exists($model, 'hit'))  // check for backend model, do not move the check above, it will fail
        {
            $model->hit();
        }
    }

[EDIT]
i have corrected function name 'hits' which should be 'hit'

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

I guess you mean "hit" instead of "hits" which does not exist...
I.e.
if (method_exists($model, 'hit')) // do not move the check above, it will fail

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

yes, sorry, function name is 'hit'

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

I let you do, folks. It can wait tonight as I guess we are not going to release a 3.5.2 because of this regression.
It would be good to add this though in the docs.

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

To be on the safe side, maybe one could do

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

although it looks like checking the layout is not really necessary.

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

yes that is even better
in the case that a future change makes pagebreak use frontend model, which would cause a hit to be counted every time the pagebreak popup is opened

@Sandra97

This comment has been minimized.

Copy link

commented Apr 6, 2016

@zero-24

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@Sandra97

This comment has been minimized.

Copy link

commented Apr 6, 2016

@zero-24, perfect! Thank you!

@GABBAR1947

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@ggppdk @infograf768 If we use is_callable() instead of method_exists() , wouldn't that be better ?
Meanwhile can I make a PR ?

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Seriously, the solution is check if a method exists!? That's frankly as much crap as the logic was in 3.5.0. If another model adds a hit method and that model is used in the display method what assurance do you have that it being called is the intended behavior when something is displayed? Even worse, since hit doesn't have a defined interface anywhere, what if a new hit method is added with parameters?

Checking on the layout is a terrible choice too (and it's already bad this is happening in the content.php file AND the controller's constructor as a means to decide ACL or what classes are included).

if article model is the frontend model

You can't do that in Joomla. ContentModelArticle is the model's class name in both site and admin. You can use Reflection to check file paths, but honestly that's not a good choice either. It shouldn't be an option since you can overload component MVC classes with plugins. We already get enough people yelling about B/C breaks when plugins stop being able to overload classes because they get pushed into the autoloader.

Honestly, we (all of us) built ourselves into a hole. Why are we using one display task to deal with the logic of a dozen view combinations when we extend and override other action tasks (add, edit, save) to fix context?

@N6REJ

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

+1

On 4/6/2016 07:45, Michael Babker wrote:

Seriously, the solution is check if a method exists!? That's frankly
as much crap as the logic was in 3.5.0. If another model adds a hit
method and that model is used in the display method what assurance do
you have that it being called is the intended behavior when something
is displayed? Even worse, since hit doesn't have a defined interface
anywhere, what if a new hit method is added with parameters?

Checking on the layout is a terrible choice too (and it's already bad
this is happening in the content.php file AND the controller's
constructor as a means to decide ACL or what classes are included).

if article model is the frontend model

You can't do that in Joomla. |ContentModelArticle| is the model's
class name in both site and admin. You can use Reflection to check
file paths, but honestly that's not a good choice either. It shouldn't
be an option since you can overload component MVC classes with
plugins. We already get enough people yelling about B/C breaks when
plugins stop being able to overload classes because they get pushed
into the autoloader.

Honestly, we (all of us) built ourselves into a hole. Why are we using
one display task to deal with the logic of a dozen view combinations
when we extend and override other action tasks (add, edit, save) to
fix context?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#9748 (comment)

@infograf768

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

@mbabker
what solution do you propose?

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

I can't say I have one that isn't one of the existing ways of checking without a B/C break but honestly the method_exists() check seems like it's the most prone to error in the future and the suggestion of checking the model is also pretty weak. At least the layout check matches up to one specific use case.

@alikon

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

why not with an event trigger like this dirty one ??
in the joomla content plugin

        public function onContentAfterDisplay($context, &$row, &$params, $page=0)
    {
        if ($context == 'com_content.article')
        {
            $table = JTable::getInstance('Content', 'JTable');
            $table->hit($row->id);
        }
        return null;
    }
`
@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Personally, I don't think core behaviors like this should be in a plugin. And if you're going to make hit counting a plugin driven function I'd say it should be more abstract than supporting a single extension.

@alikon

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

....feedback is warmly wellcommed ...

p.s.
as usual my quick&dirty fix ☺️

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

See #9768

@brianteeman brianteeman closed this Apr 6, 2016

@alikon

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

@brianteeman do you mean #9766 ?

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2016

yes - oops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.