Skip to content

Correcting Notice when saving Global Configuration#13612

Merged
wilsonge merged 2 commits intojoomla:stagingfrom
infograf768:cache_path_notice
Jan 20, 2017
Merged

Correcting Notice when saving Global Configuration#13612
wilsonge merged 2 commits intojoomla:stagingfrom
infograf768:cache_path_notice

Conversation

@infograf768
Copy link
Copy Markdown
Member

@infograf768 infograf768 commented Jan 16, 2017

Pull Request for Issue #13608

When saving Global Configuration and Path to Cache Folder is empty, we get a Notice in the PHP logs:
[16-Jan-2017 16:07:25 UTC] PHP Notice: Undefined index: cache_path in /administrator/components/com_config/model/application.php on line 290

This came from the merged PR #13520

Test before and after patch.

@mbabker

@ghost
Copy link
Copy Markdown

ghost commented Jan 16, 2017

With and without Patch didn't get a Notice.
bildschirmfoto 2017-01-16 um 20 19 08

@infograf768
Copy link
Copy Markdown
Member Author

@franz-wohlkoenig
Have you looked at the php_error.log file as this Notice is not going to display in the browser window?

@joomdonation
Copy link
Copy Markdown
Contributor

The easiest way to see error is adding this command

die();

after this line https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_config/model/application.php#L387

@infograf768

I think $data['cache_path'] is always available, so actually, to prevent the notice, we only have to check !empty($prev['cache_path'])

So the code would become:

if ($data['cache_path'])
{
	$path = $data['cache_path'];
}
elseif (!$data['cache_path'] && !empty($prev['cache_path']))
{
	$path = $prev['cache_path'];
}
else
{
	$path = JPATH_SITE . '/cache';
}

@joomdonation
Copy link
Copy Markdown
Contributor

joomdonation commented Jan 17, 2017

Or even simpler

if ($data['cache_path'])
{
	$path = $data['cache_path'];
}
elseif (!empty($prev['cache_path']))
{
	$path = $prev['cache_path'];
}
else
{
	$path = JPATH_SITE . '/cache';
}

I don't see the need for checking !$data['cache_path'] in else if block.

@ghost
Copy link
Copy Markdown

ghost commented Jan 17, 2017

I have tested this item ✅ successfully on b344bf3


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

@ghost
Copy link
Copy Markdown

ghost commented Jan 17, 2017

thanks for helping, @joomdonation

@joomdonation
Copy link
Copy Markdown
Contributor

You're welcome @franz-wohlkoenig . If @infograf768 agree with my suggested changes, we will need your help re-test this PR again.

@ghost
Copy link
Copy Markdown

ghost commented Jan 17, 2017

will do.

@infograf768
Copy link
Copy Markdown
Member Author

@franz-wohlkoenig @joomdonation Simplified as suggested, please test again.

@joomdonation
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on b069f82


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

@joomdonation
Copy link
Copy Markdown
Contributor

@infograf768 Thanks for making the changes. I marked the test result.

@ghost
Copy link
Copy Markdown

ghost commented Jan 19, 2017

  1. Tested with PR got a blank Site,
  2. tested without PR got Notice: Undefined index: cache_path in /var/www/web17/html/Joomla/administrator/components/com_config/model/application.php on line 290

Maybe cause couldn't install todays nightly build (Download of update package failed.).

@joomdonation
Copy link
Copy Markdown
Contributor

Blank site? Before you press save or after pressing save? Maybe it is blank because you added die(); command?

@ghost
Copy link
Copy Markdown

ghost commented Jan 19, 2017

After pressing save. Maybe because of die(); but to see the Notice isn't it helpful?

@joomdonation
Copy link
Copy Markdown
Contributor

Yes. The die(); command is used to allow you to see notice before patch. After patch, you see blank screen means no notice anymore

So for now, you should remove the die(); command, then save the configuration again. If configuration (especially cache path) is being saved, then you can mark the test result as success

@ghost
Copy link
Copy Markdown

ghost commented Jan 19, 2017

I have tested this item ✅ successfully on b069f82


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

@ghost
Copy link
Copy Markdown

ghost commented Jan 19, 2017

@joomdonation haven't known that there's a beta1 and custom-path-update didn't work. After installing beta1 test was sccessfully.

@joomdonation
Copy link
Copy Markdown
Contributor

joomdonation commented Jan 19, 2017

Sorry, I was wrong. The code looks OK

@infograf768
Copy link
Copy Markdown
Member Author

@joomdonation
I suggest you create a new PR as this one deals only with the Notice. And tag @mbabker as he is the one who created the original PR.
I mark this one as RTC

@infograf768
Copy link
Copy Markdown
Member Author

RTC. Thanks.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 19, 2017
@wilsonge wilsonge merged commit f2d72e5 into joomla:staging Jan 20, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 20, 2017
@wilsonge wilsonge added this to the Joomla 3.7.0 milestone Jan 20, 2017
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.

4 participants