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

Template Customization Doesn't Apply #12651

Closed
Espionage724 opened this issue Oct 31, 2016 · 26 comments
Closed

Template Customization Doesn't Apply #12651

Espionage724 opened this issue Oct 31, 2016 · 26 comments

Comments

@Espionage724
Copy link

Espionage724 commented Oct 31, 2016

Steps to reproduce the issue

  1. Go to template customization (like Protostar)
  2. Change settings (like colors or Fluid vs Static)
  3. Apply changes

Expected result

  • Changes apply to main site

Actual result

  • Changes do not apply

System information (as much as possible)

  • Joomla from Git (staging branch; latest as of 24 hours ago)
  • Ubuntu 16.10 with PHP7
  • nginx

Additional comments

  • This happens with the default Protostar template, Breeze3, T3 Framework (both blank templates), and Purity III
  • This only affects Joomla from Git (latest 3.6.4 release works fine)
@SharkyKZ
Copy link
Contributor

Caused by #12395.

@brianteeman
Copy link
Contributor

I can not confirm this with the latest staging

Please can you reconfirm


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

@Espionage724
Copy link
Author

It still doesn't seem to work for me. I pull from staging, go through initial set-up, and then immediately go over to Template > Styles > protostar - Default > Edit Style.

I change the Template Colour and Fluid Layout, Save, but the changes don't take effect on the main site.

@zero-24
Copy link
Contributor

zero-24 commented Oct 31, 2016

Can you please double check that there is no JS cached?

@Espionage724
Copy link
Author

How could I check? I don't have the cache enabled from Global Configuration in Joomla so I'm not certain if JS would be cached from there. I've done a full page refresh (Ctrl + F5) but it didn't change anything.

@zero-24
Copy link
Contributor

zero-24 commented Oct 31, 2016

If you use chrome you can disable caching in the development console.

@Espionage724
Copy link
Author

Ah; nope disabling caching from the browser doesn't seem to change anything.

@Espionage724
Copy link
Author

Ah, I figured it out; it looks like there's no menus assigned by-default for the theme to affect. If I assign it to the Main Menu, changes apply without issue.

@brianteeman
Copy link
Contributor

Glad you worked it out.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Nov 1, 2016

This is still incorrect. Please reopen.
Default style is applied to all pages by default. Having to select specific menu items is unexpected behavior. The issue is caused by #12395. The result of $app->getTemplate(true)->params; inside templates changes with this PR causing incorrect behavior. @mbabker, can you take a look at this?

@brianteeman
Copy link
Contributor

I still cannot replicate this

On 1 November 2016 at 13:39, SharkyKZ [email protected] wrote:

This is still incorrect. Please reopen.
Default style is applied to all pages by default. Having to select
specific menu items is unexpected behavior. The issue is caused by #12395
#12395. The result of
$app->getTemplate(true)->params; inside templates changes with this PR
causing incorrect behavior. @mbabker https://github.com/mbabker, can
you take a look at this?


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#12651 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8cF-zuWPrKmA-fAno3TnmZyjSScVks5q50ESgaJpZM4KktKk
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

@mbabker
Copy link
Contributor

mbabker commented Nov 1, 2016

If you can show how that's the issue I'd be glad to look further. But new Registry($data); and $registry = new Registry; $registry->loadString($data); are functionally the same code so how making that change on its own causes the issue I don't see (and I'm honestly on a tight schedule so I don't have a lot of time to chase things like this down right now, sorry).

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Nov 1, 2016

To replicate, do a fresh install of staging or 3.7. Change some param in Protostar style. See that nothing actually changes. None of the params work.

Var dump of $app->getTemplate(true)->params; before PR:

object(Joomla\Registry\Registry)#247 (2) {
  ["data":protected]=>
  object(stdClass)#248 (8) {
    ["templateColor"]=>
    string(7) "#000000"
    ["templateBackgroundColor"]=>
    string(7) "#f4f6f7"
    ["logoFile"]=>
    string(0) ""
    ["sitetitle"]=>
    string(0) ""
    ["sitedescription"]=>
    string(0) ""
    ["googleFont"]=>
    string(1) "1"
    ["googleFontName"]=>
    string(9) "Open+Sans"
    ["fluidContainer"]=>
    string(1) "1"
  }
  ["separator"]=>
  string(1) "."
}

after PR:

object(Joomla\Registry\Registry)#247 (2) {
  ["data":protected]=>
  object(stdClass)#248 (2) {
    ["data"]=>
    object(stdClass)#249 (8) {
      ["templateColor"]=>
      string(7) "#000000"
      ["templateBackgroundColor"]=>
      string(7) "#f4f6f7"
      ["logoFile"]=>
      string(0) ""
      ["sitetitle"]=>
      string(0) ""
      ["sitedescription"]=>
      string(0) ""
      ["googleFont"]=>
      string(1) "1"
      ["googleFontName"]=>
      string(9) "Open+Sans"
      ["fluidContainer"]=>
      string(1) "1"
    }
    ["separator"]=>
    string(1) "."
  }
  ["separator"]=>
  string(1) "."
}

A stdClass object named data is added and params are now contained within it.

@mbabker
Copy link
Contributor

mbabker commented Nov 1, 2016

This might be a case similar to one Thomas identified where an existing Registry object is being bound to another one, which can cause issues. Especially as the Registry constructor has logic to handle objects (which IIRC is triggered before the string handling code).

So not necessarily an issue with that PR, but it exposes we have some cases where we are taking an existing Registry, converting it to a string, then loading it into a new Registry. Woefully inefficient.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Nov 1, 2016

Whatever it is, it's a release blocker.

@brianteeman
Copy link
Contributor

brianteeman commented Nov 1, 2016 via email

@dgrammatiko
Copy link
Contributor

Hint: router

@mbabker
Copy link
Contributor

mbabker commented Nov 1, 2016

I don't think it's the router, I'm honestly not sure how to replicate it either. I do get the feeling though it's within JApplicationSite::getTemplate(), as @SharkyKZ keeps pointing out, specifically this blob as that's the only place where a Registry object is being created.

Since the foreach loop inside that blob modifies each object by reference, it's possible that the params property of the $template object is already a Registry object. So new Registry($existingRegistry); will cause the Registry constructor to bind the existing Registry's data onto the new object and could result in the nesting shown above. In this case you should be doing $newRegistry->merge($existingRegistry);.

Now, with that said, there's also an off chance something that calls JApplicationSite::setTemplate() is binding bad data. But core itself doesn't make any calls to that method, so if this can be replicated with a core install only it has nothing to do with that.

@Espionage724
Copy link
Author

With a clean-install of Joomla 3.6.4 and git releases prior to maybe a few weeks ago, Protostar would be selected by-default, and any changes applied to it would just-work without having to do anything else. And in my case, switching to Purity III template, I'd just click the Star in ACP. Under the template's Assign menu, no items are checked by-default, but this doesn't seem to matter (changes still apply site-wide).

With current git Joomla, the template isn't applied to any menu items by default, and clicking the Star to use a specific template doesn't just-work either. I have to also go under the template's configuration, go under Assignment, and select the menu items I want to have affected.

@brianteeman
Copy link
Contributor

If a template is marked as Default it does not need to be assigned to anything not should it be. Default means it is assigned to everything. You only assign a template to a menu item if you want to override the default template for that specific menu item

Stating all of that I still cannot replicate the issue reported here of changes to a style not being used. Please provide step by step instructions how a change made to a template style is not presented on a site when that template style is either the default template or a template assigned to a specific menu.

At this time as it is completely unclear what this issue is I am removing the "release-blocker" tag.

@brianteeman
Copy link
Contributor

(oops someone beat me to that tag removal)

@csiteru
Copy link
Contributor

csiteru commented Nov 1, 2016

Hi! I have some problem with styling T3 template after update to J3.6.4. All my styles of site for different menu - default.


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

@ggppdk
Copy link
Contributor

ggppdk commented Nov 1, 2016

Since the foreach loop inside that blob modifies each object by reference, it's possible that the params property of the $template object is already a Registry object.

It is happening because we are modifying $templates to add 1 more element to the loop, that is the:
$templates[0] which is home template

https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/application/site.php#L488

Thus the foreach loop will do 1 more execution for the newly added array element, but parameters are already an object. I think my PR: #12688 fixes it

@zero-24
Copy link
Contributor

zero-24 commented Nov 2, 2016

Closing as there is a PR. Thanks!

@zero-24 zero-24 closed this as completed Nov 2, 2016
@csiteru
Copy link
Contributor

csiteru commented Nov 2, 2016

Hmmm... I have not changed with the latest madifications in #12688. All T3 templates have default settings

@ggppdk
Copy link
Contributor

ggppdk commented Nov 2, 2016

They may have similar problem like what my PR fixes in their custom code

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

9 participants