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

3.7: Fatal error due to #11225 #12095

Closed
infograf768 opened this issue Sep 20, 2016 · 12 comments
Closed

3.7: Fatal error due to #11225 #12095

infograf768 opened this issue Sep 20, 2016 · 12 comments

Comments

@infograf768
Copy link
Member

Steps to reproduce the issue

Load the articles manager
Fatal error: Can't use method return value in write context in ROOT/administrator/components/com_content/models/articles.php on line 329

@infograf768
Copy link
Member Author

Looks like it would be solved by using ! instead of empty? I.e.

if (JPluginHelper::isEnabled('content', 'vote'))
        {
            $orderCol  = !($this->state->get('list.fullordering', 'a.id')) ? 'a.id' : $this->state->get('list.fullordering', 'a.id');
            $orderDirn = '';
        }

@alikon
Copy link
Contributor

alikon commented Sep 20, 2016

#11225 has been tested on 3.6.x branch not on 3.7 branch I'll look at it
with the 3.7...

On 20 Sep 2016 9:13 am, "infograf768" notifications@github.com wrote:

Looks like it would be solved by using ! instead of empty? I.e.

if (JPluginHelper::isEnabled('content', 'vote'))
{
$orderCol = !($this->state->get('list.fullordering', 'a.id')) ? 'a.id' : $this->state->get('list.fullordering', 'a.id');
$orderDirn = '';
}


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#12095 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALFsSHLLKwSw_YHHl6x80Khld8x0_Gfks5qr4e0gaJpZM4KBSgQ
.

@sovainfo
Copy link
Contributor

Seriously?

$orderCol = !($this->state->get('list.fullordering', 'a.id')) ? 'a.id' : $this->state->get('list.fullordering', 'a.id');

for
$orderCol = $this->state->get('list.fullordering', 'a.id');

@infograf768
Copy link
Member Author

What we have now and causes the Fatal error is:

        if (JPluginHelper::isEnabled('content', 'vote'))
        {
            $orderCol  = empty($this->state->get('list.fullordering', 'a.id')) ? 'a.id' : $this->state->get('list.fullordering', 'a.id');
            $orderDirn = '';
        }

@sovainfo
Copy link
Contributor

Indeed, should never have been accepted!

@ggppdk
Copy link
Contributor

ggppdk commented Sep 20, 2016

How many people test with PHP 5.3 and 5.4 ?
empty() in these versions, works on variable only

@infograf768
Copy link
Member Author

My test was on php 5.4.4

@mbabker
Copy link
Contributor

mbabker commented Sep 20, 2016

All you need is the line @sovainfo pointed out, $orderCol = $this->state->get('list.fullordering', 'a.id');. If you have to check if the state's value is empty after you've pulled from the object then you've royally screwed something up along the way. If it can't be empty don't allow an empty value to be saved to the state!

@ggppdk
Copy link
Contributor

ggppdk commented Sep 20, 2016

@infograf768

My test was on php 5.4.4

yes i understood that because you got the error,
but i meant not your test , i meant the 2 human "acceptance" tests on every PR

@alikon
Copy link
Contributor

alikon commented Sep 20, 2016

@infograf768 i'm unable to reproduce on PHP Version 5.5.30

just want to remember that still minimum Requirements for Joomla! 3.x is 5.3.10

and yes should be $orderCol = $this->state->get('list.fullordering', 'a.id'); my mistake
i'll fix it

@sovainfo
Copy link
Contributor

People are known to make mistakes, procedures are in place to avoid mistakes getting into the product. That doesn't mean every mistake is caught! Luckily, this one was.

Of the almost 500 $this->state->get statements in the product a few also use escape() around it. Don't know whether it needs it.

@zero-24 zero-24 removed this from the Joomla 3.7.0 milestone Sep 20, 2016
@zero-24
Copy link
Contributor

zero-24 commented Sep 20, 2016

Closing as there is a PR. Thanks!

@zero-24 zero-24 closed this as completed Sep 20, 2016
wilsonge added a commit that referenced this issue Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants