Skip to content

[com_actionlogs] - fix non object notice#22526

Merged
wilsonge merged 1 commit intojoomla:stagingfrom
alikon:patch-94
Oct 8, 2018
Merged

[com_actionlogs] - fix non object notice#22526
wilsonge merged 1 commit intojoomla:stagingfrom
alikon:patch-94

Conversation

@alikon
Copy link
Contributor

@alikon alikon commented Oct 7, 2018

Pull Request for Issue #22525

Testing Instructions

see #22525

Expected result

no notice

Actual result

PHP error log
PHP Notice: Trying to get property 'id' of non-object

@Quy
Copy link
Contributor

Quy commented Oct 7, 2018

I have tested this item ✅ successfully on 49778af


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22526.

1 similar comment
@infograf768
Copy link
Member

I have tested this item ✅ successfully on 49778af


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22526.

@infograf768
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22526.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 8, 2018
@infograf768 infograf768 added this to the Joomla 3.9.0 milestone Oct 8, 2018
@infograf768
Copy link
Member

Folks, please test #22518
That also happens when editing a module in frontend.

@wilsonge wilsonge merged commit 036dff8 into joomla:staging Oct 8, 2018
@joomla-cms-bot joomla-cms-bot added PR-staging and removed RTC This Pull Request is Ready To Commit labels Oct 8, 2018
@alikon alikon deleted the patch-94 branch October 8, 2018 17:58
protected function addLog($messages, $messageLanguageKey, $context, $userId = null)
{
$user = $this->app->getIdentity();
$user = JFactory::getUser();
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if we have a bug here? Why is getIdentity null?

Copy link
Contributor

Choose a reason for hiding this comment

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

For 3.x? Because loadIdentity is not consistently called.

Copy link
Member

Choose a reason for hiding this comment

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

So this needs to be fixed then. As far as I know in 4 it works reliably.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was a specific fix for that in 4.x we always call it early in the stack to ensure it's loaded (#16398). It's not something I'd feel comfortable porting back into 3.x because it might well affect anything that extends CMSApplication

Copy link
Member

Choose a reason for hiding this comment

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

Actually it got fixed in #19766. It would make sense to port it back to 3 or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well then is #16398 actually doing anything. Because that should have been enough on it's own

If we do backport it would have to be 3.10 and would need some heavy testing because it might well have affects on things that extends CMSApplication (i know when michael made it a final class some people complained and it ended up being reverted so there are definitely people doing that)

Copy link
Contributor

Choose a reason for hiding this comment

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

It is more involved than those two patches. You're going to have to dig back even deeper to when the session API was refactored to use the Framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants