When I try to change the theme directly in the edit page of a node, the assign node theme module don't work. The theme don't change and I have the admin 1 role.

Must I change something in the section that I want to assign to my node ? checkboxes ?

Comments

hass’s picture

Status: Active » Postponed (maintainer needs more info)

What is "node theme module"?

pixelpreview@gmail.com’s picture

sorry it's not a module but a functionnality
I have this in my edit page

".. theme configuration
This setting allows you to create a section per node. A node section will get the highest weight and take precedence about all other inheriting sections.
Select theme : ( a menu list of my themes )

Select the theme you want to use for this node. Disabled themes are not used until they are enabled on themes page.
.."

and in permissions page, I see that :

module sections
> administer sections (checkbox)
> assign node theme (checkbox) <-----

I think that's that functionnality "assign node theme" that I see in my edit page no ?

hass’s picture

This must work, but it's a new sections feature.

Wether the selected theme is disabled or i don't know. You need to debug. Have you upgraded from earlier version? Make sure you run update.php. Are the node theme setting kept? Keep in mind thatonly the node view may change.

pixelpreview@gmail.com’s picture

the selected theme is not disabled because I use it with the section module to assign it to some paths and it works without problems
It seems that these functionnality has no effects on my node when I try to assign a theme directly in the node
The section module works but not these function certainly a conflict with an another module
thanks for your feedback

hass’s picture

Have you found out the reason?

pixelpreview@gmail.com’s picture

no and I use only section module
These function doesn't work and actualy I don't have found the solution
My site is a portal with 10 sections and 10 subthemes in css I think that the problem is perhaps here
a conflict in the weight of a theme ? I don't know

hass’s picture

Can you try the next DEV, please? Enable the new debugging setting and go to your failing node to see what's going on. Come back with the debugging information, please.

mrfelton’s picture

Status: Postponed (maintainer needs more info) » Active

If a node matches the path spec from a sections_data definition AND has a node specific theme definition, the sections_data definition will take precedence. I think this is the wrong behavior.

Node specific theme definition should override other sections definitions.

I think this happens because the sections_data theme switching happens in hook_init.

init_theme() can only be called once.

So, if hook_init() switches the theme, then the node specific theme switching in hook_nodeapi doesn't work.

mrfelton’s picture

Category: support » bug
mrfelton’s picture

Status: Active » Needs review
StatusFileSize
new1.11 KB

Here is a patch.

Rather than using nodeapi, it's all done in in hook_init.

If you are viewing a node, then it checks to see if the node has a specific theme selection, and if so it applies the theme and returns.

If no theme was found, or you were not viewing a node, then the defined sections are checked.

mrfelton’s picture

Here is one without the typo in the comment too!

hass’s picture

Priority: Normal » Critical

Blocking release

hass’s picture

@mrfelton: Does if (arg(0) == 'node' && is_numeric(arg(1)) && !arg(2)) { work if the node have an alias? I guess no, isn't it?

mrfelton’s picture

@hass - yes, it does. It is a very standard way of checking if you are on a node view page.

mrfelton’s picture

Here is an enhancement version that caters for node preview and revisions pages

if (arg(0) == 'node' && is_numeric(arg(1)) && ( !arg(2) || arg(2) == 'preview' || (arg(2) == 'revisions' && arg(4) == 'view') ) ) {

hass’s picture

Status: Needs review » Needs work

This cannot work in a general manner. Code need to move into _sections_in_section() or it does not return a node specific theme if other modules like to know the section they are in.

hass’s picture

Title: assign node theme do'nt work when I choice directly the theme in the node edit page » Node theme don't take precedence over path based sections
Version: 6.x-1.4 » 6.x-1.x-dev
Component: User interface » Code
Status: Needs work » Needs review
StatusFileSize
new5.48 KB

New patch for review.

Re #15: I see your point for this, but it's not the standard core way, therefore left this out.

mrfelton’s picture

@hass, checking arg() is the standard core way of checking if you are on a node edit/view page. Core doesn't have any situations where it needs to check if you are viewing a revision, and so it never does (arg(2) == 'revisions' && arg(4) == 'view') ). But, we do need to do that check, because unlike core, we do need to do something specific if people are looking at the node revisions tab - we need to switch the theme there if appropriate. Otherwise, it is not possible to view your revisions with node specific section overrides.

mrfelton’s picture

Note that I also included checking arg(2) == 'preview', however, node/*/preview is not really a standard path (unlike node/*/revisions/*/view) - I have a custom module that alters the node preview button, so that it pops up the preview in a new browser tab, and a custom mennu callback that is able to show the node preview at the url node/*/preview. I'd accept leaving that part out, since that only applies in my situation (with my custom module installed). However, the revisions things is a standard path, that is part of the drupal core, and I really think that node revisions should be shown in the correct section theme.

hass’s picture

Hm... other modules may also show a tab in a node... for this case we need to change to if (arg(0) == 'node' && is_numeric(arg(1))) {. But over all it may be bad, too as the administration theme for edit tab is not working... very bad... endless collisions... but we could argue that this is a negative side effect that is at least acceptable here.

We need to rethink if the node part should not better be rewritten and all sections_nodes table data move into to sections_data to make this more generic. In this case the path's become configurable like all other sections path's, too.

Something we could keep in mind and fix in the next version.

hass’s picture

#18: I may was unclear what I'm talking about... in core we have this feature named "Use administration theme for content editing". I tried to replicate this in sections, but here it fails. We cannot add all possible arguments and verify them, this is why we may need to stay with #20 for now.

hass’s picture

Assigned: Unassigned »
Status: Needs review » Needs work

I don't like my patch. I will rewrite it and add all node themes to the old sections_data table. This will reduce complexity and allow changing the path filters for node themes, too.

callison’s picture

Title: Node theme don't take precedence over path based sections » Node theme doesn't take precedence over path based sections
Status: Needs work » Needs review
StatusFileSize
new957 bytes

Since there's been no action on here for several months, I thought I'd go ahead. First, a couple of thoughts:

  1. IMHO, hass' approach is too broad - not focusing on just the issue at hand and...
  2. mrfelton's approach is a bit redundant because the functionality is already there in hook_nodeapi

I've created a new patch here and what I've done is simply add a little section in hook_init that checks if the current node (if we're on a node) has a theme setting and, if so, doesn't allow the section theme to load so that hook_nodeapi can load it later. Therefore this fixes the problem at hand (including hass' thoughts about previews, revisions, etc) and does not change the way the module works (which, IMO should be saved for another issue). I'd be interested in your feedback.

hass’s picture

This is like mfeltons and break in many ways. It's tooo simple.

callison’s picture

@hass: Can you please explain how it breaks? Thanks.

[Edit] I understand what you were saying in #16, but couldn't we then just add an (almost identical) section of code (or a new function or something) in _sections_in_section() that will check if we're on a node and return that node's theme settings if they're available? What I was understanding about your patch is that it is trying to change the whole way this module handles the node theming (using the node caches and stuff) and that seems unnecessary to me. Please give me your thoughts. Thanks.

BTW, I'm not saying that your approach is bad - just IMO it's not a necessary change for now and should be integrated into a next release or something instead of as a fix for this patch. Just a thought. Let me know what you think.

hass’s picture

Status: Needs review » Needs work

If someone have configured to show node edit pages with admin theme it will break. There are several details that also not working for many other people.

callison’s picture

Alright. Well, is there anything I can do that would help get it right? Do you want to provide some use cases or something and talk out a good solution? I'm here to help if you want me.

mrfelton’s picture

Dont think you should be calling node_load in hook_init.

mrfelton’s picture

Status: Needs work » Needs review
StatusFileSize
new785 bytes

Here is the same patch as #23, but doing a simple db look up in hook_init instead.

mrfelton’s picture