Needs review
Project:
Build modes
Version:
6.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
25 May 2011 at 11:06 UTC
Updated:
5 Jun 2011 at 12:40 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
plachThe attached patch does it :)
Comment #2
Anonymous (not verified) commentedThis is being triggered on $op = 'view', which assumes that buildmodes module is weighted lower than other modules that want to affect the node content array. I realize that CCK is where build modes are used, and fields won't be added until content_view is run, but build mode should be set as soon as possible after the node is loaded.
Could the context reaction be executed when $op = 'load' so that the build mode is set correctly at the very beginning of the 'view' op?
Thanks for the contribution. I think it can go in after we answer this question ... either way.
Comment #3
plachWouldn't we risk to alter build modes in a phase that might impact code relying on unaltered build modes? I mean, we load nodes before saving them, we cannot assume we are in a view workflow.
Comment #4
Anonymous (not verified) commentedThat's true. The build mode only has an effect during $op = 'view' and $op = 'alter' and it's not saved with the node... it should be safe to node_load, set a build mode, and node_save because nothing should save it.
But the $a3 and $a4 parameters of hook_nodeapi are only passed in during the view operations, and this is how CCK sets the build mode of 'normal' nodes that don't have a special build mode.
I think that answers the question then. Doing this in the load operation looks like a WTF rather than a good solution. Let me review the patch again and I'll hopefully commit it soon.
Comment #5
Anonymous (not verified) commentedI'm not very keen on changing the module weight. "buildmodes" will always run before "content" because of alphabetical order. That should be removed unless you can explain why it's needed.
The plugin file's execute method has a comment:
It would be helpful to explain in the comment why you're doing additional checks. Is this a normal thing for Context plugins to have to check each others' conditions? (If it is, just tell me where I can see another case of it so I can refer to it in the code comment.)
If you can share your use case for this plugin, I can add it to the README so people are inspired to use it correctly. I'm guessing: mobile sites?
Comment #6
plachI can test this more, but I had to introduce that weight alteration to make everything work. I'll confirm if this is actually needed or if I missed something.
I ain't sure this is a normal thing, I'll provide a more meaningful explanation. If I'm not mistaken, usually reactions don't check whether conditions are met since it's responsibility of the code executing them to check conditions. We do it here since we might have multiple active contexts and so we have to check the node condition for each one.
It's a pretty simple use case: I have small site where every page has slideshow of images, each one filling the entire page width. In the homepage we still have a slideshow, but it fills a smaller area of the page so I need a different imagecache preset. This plugin lets me override the full build mode with a custom one in which I can set the different imagecache preset, just for the homepage, without additional lines of PHP in the theming layer: everything is setup through the UI.
Comment #7
plachChecked the weight issue: the problem is that I'm using a custom installation profile and since the modules list is sorted by filename buildmodes hooks are executed after content ones. I'm afraid there is no alternative to make this work.
I improved the comments about the node type condition.
Comment #8
Anonymous (not verified) commentedI agree on the need to change buildmodes' module weight. I wonder though how your profile can work effectively with CCK if it conflicts with buildmodes. What's the name of your profile?
The comment looks good.
I'll commit it today!
Comment #9
plachIt's not an official profile, these are my file names:
It is the first time I use buildmodes, probably it wouldn't work without that weight alteration.
Thanks!
Comment #10
plachComment #11
plachSmall followup: plainly enforcing the node type condition causes a wrong behavior with multiple conditions if OR mode is enabled. The attached patch should fix it.