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

Regression 3.7: last template style preview not working #1 #13775

Merged
merged 5 commits into from
Feb 2, 2017
Merged

Regression 3.7: last template style preview not working #1 #13775

merged 5 commits into from
Feb 2, 2017

Conversation

joeforjoomla
Copy link
Contributor

@joeforjoomla joeforjoomla commented Jan 27, 2017

Reference #13637

Steps to reproduce the issue

Install a new template on Joomla 3.7

Try to open the preview using the URL such as:
http://joomla37/?template=mytemplate

Expected result

The frontend uses the latest installed template named 'mytemplate'

Actual result

The default template is still used

System information (as much as possible)

The code change in site.php included in the function 'getTemplate' caused the regression.

An additional line of code 510 must be added in order to unset the reference to the array before reassigning it:

// Unset the $template reference to the last $templates[n] item cycled in the foreach above to avoid to edit the $templates array in the following assignment
unset($template);

Additional comments

@infograf768
Copy link
Member

you are modifying parts of this pr #12688 please make sure you do not recreate the issue it solved.

@joeforjoomla
Copy link
Contributor Author

Probably it would be better fix the new code #12688 instead of reverting to the previous one that i posted

@mbabker
Copy link
Contributor

mbabker commented Jan 27, 2017

It looks like you are reverting changes from several pull requests and moving back to the 3.6.5 state. Please don't do that, only make the changes needed to fix the issue. As I commented on your issue last week that function has had four separate pull requests applied to it when comparing 3.6.5 to current staging.

@joeforjoomla
Copy link
Contributor Author

joeforjoomla commented Jan 27, 2017

Add line 510

`// Unset the $template reference to the last $templates[n] item cycled in the foreach above to avoid to edit the $templates array in the following assignment

unset($template);`

@@ -507,6 +507,8 @@ public function getTemplate($params = false)
$cache->store($templates, $cacheId);
}

// Unset the $template reference to the last $templates[n] item cycled in the foreach above to avoid to edit the $templates array in the following assignment
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done in the else part of the above if/else. $template isn't defined if the data is pulled out of the cache.

@mbabker
Copy link
Contributor

mbabker commented Jan 28, 2017

OK, I moved the check myself. This PR looks fine to me, tests please.

@ghost
Copy link

ghost commented Jan 28, 2017

I have tested this item ✅ successfully on 796a057


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

1 similar comment
@marrouchi
Copy link
Contributor

I have tested this item ✅ successfully on 796a057


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

@infograf768
Copy link
Member

RTC. Thanks


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

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

6 participants