Clearing cache on Skin settings page (admin/appearance/skinr/skins/settings/%) causes errors default groups from skinr_skinr_group_info() to not get loaded. The errors are evident as notices on Skins listing page (admin/appearance/skinr/skins).

Files: 
CommentFileSizeAuthor
#33 patch_commit_7c831e83210e.patch106.44 KBmoonray
#32 patch_commit_32086bf9fe14.patch37.37 KBmoonray
#30 patch_commit_26cbc0bdeda8.patch15.78 KBmoonray
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_26cbc0bdeda8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 patch_commit_077228b11a02.patch11.71 KBmoonray
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_077228b11a02.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 patch_commit_a899d8ae3f71.patch93.5 KBmoonray
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_a899d8ae3f71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 patch_commit_db21c248c1be.patch84.39 KBmoonray
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_db21c248c1be.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 patch_commit_13d6bab80e2e.patch93.75 KBmoonray
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_13d6bab80e2e.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 skinr.kill-init.19.patch11.26 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 skinr.kill-init.18.patch10.94 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 skinr.kill-init.17.patch9.78 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 skinr.kill-init.16.patch8.93 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 skinr.kill-init.11.patch1.43 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 skinr.kill-init.10.patch937 bytessun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

moonray’s picture

This looks to be due to skinr_ui_skin_info_load() being run before skinr_init().

sun’s picture

Before hook_init() means before full bootstrap. This should not happen.

Add a ddebug_backtrace() to skinr_ui_skin_info_load() to see who or what is attempting to theme() something before full bootstrap.

moonray’s picture

This is what the backtraces tell me:

  • Bootsrap is run.
  • Line 4935 of common.inc: menu_set_custom_theme() eventually runs skinr_ui_skin_info_load().
  • Line 4937 of common.inc: module_invoke_all('init') is run.

It looks legit, but it doesn't give us the desired execution order.

sun’s picture

mmm, I don't see how or where menu_get_custom_theme() would or could invoke skinr_ui_skin_info_load(). There must be some intermediary function(s) in between, no? Can you attach a full debug_backtrace() as a .txt file attachment?

(Ideally, we'd have a function in core that allows to output a simplified backtrace; reminds me to continue work on #1158322: Add backtrace to all errors)

moonray’s picture

Here are the backtrace functions:

1. drupal_bootstrap [line: 21]
2. _drupal_bootstrap_full [line: 2056]
3. menu_set_custom_theme [line: 4935]
4. menu_get_custom_theme [line: 1689]
5. menu_get_item [line: 1674]
6. _menu_translate [line: 451]
7. _menu_load_objects [line: 744]
8. skinr_ui_skin_info_load [line: 577]

I've also attached the full backtrace.

moonray’s picture

EDIT: removed double post.

ChrisBryant’s picture

I'm not sure why, but for some reason the link to the uploaded backtrace.txt file has a path of just "http://drupal.org/files/issues/" which makes it not link. The file appears to have been uploaded though:

http://drupal.org/files/issues/backtrace_12.txt

Unfortunately I can't offer any further help with this issue. :-)

sun’s picture

See end of _drupal_bootstrap_full().

This is more or less expected behavior, as far as I can see.

However, this means that hook_init() was not invoked yet, but all modules are loaded already. So I don't really understand why Skinr has a problem with that, and why default groups from skinr_skinr_group_info() cannot be loaded?

moonray’s picture

skinr_load_includes() which loads the *.skinr.inc (where skinr_skinr_group_info() resides) is run by skinr_init().
So, unless hook_init() is run, skinr_skinr_group_info() doesn't exist, and a version of loaded groups without our default skinr groups is cached.

sun’s picture

Status:Active» Needs review
StatusFileSize
new937 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

well, there's a big phat @todo for skinr_init() which deserves to be resolved in the first place. ;)

sun’s picture

StatusFileSize
new1.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And a similarly big one for skinr_load_includes().

All of this manual, unconditional loading of each and every Skinr support file shouldn't be required anymore. That's why we revamped Skinr's plugin API. If it's still needed for any reason, then we need to fix that.

moonray’s picture

Status:Needs review» Needs work

You've left out skinr_load_includes() in your patch, which loads modules/block.skinr.inc, etc. They can't be loaded using module_load_include('inc', 'skinr', 'skinr.handlers') because they're not in their own modules' dirs. That was the reason we left in the functions that run on hook_init() in the first place.

sun’s picture

The already existing skinr_implements() (and skinr_load_plugins()) should auto-load everything that is needed, and only if it is needed.

The global, unconditional loading of include files needs to go. For that sake, the built-in API hooks for core modules already register themselves with:

/**
* Helper function for built-in integration code.
*/
function skinr_skinr_api_modules() {
  return array(
    'path' => drupal_get_path('module', 'skinr') . '/modules',
  );
}

function block_skinr_api_2() {
  return skinr_skinr_api_modules();
}

In turn, this should work already. If it doesn't, we need to find out why, and fix it.

moonray’s picture

This actually only works on the first run (due to caching). After that the files aren't loaded...

e.g. for modules/block.skinr.inc, block_skinr_config_info() is run and cached when skinr_implements() is run. But the function callbacks registered in that module probably need to be standardized so they can be included skinr_hook_info() to ensure auto-loading when required.

EDIT: #1044222: Remove skin configuration functionality from third-party forms, thought closed has pertinent information.

moonray’s picture

Since we're going to have to deal with the API changes to get this bug fixed, I'm reposting relevant info from #1044222: Remove skin configuration functionality from third-party forms comments #16 and #18.

by @moonray:
We have the following callbacks left after this last patch:

preprocess_hook_callback
Returns a list of all theme hooks (hooks used in the preprocess function) that this element could apply to. The given context is $form and $form_state from hook_form_alter().

[EDIT] The reason we're using $form and $form_state as arguments is due to legacy functionality; we were adding this form to third party edit forms. We can most likely streamline this now. This callback is called from within skinr_ui_form_alter() which adds creates the widgets form skinr plugin provided skins and groups.

index_handler for preprocess function
Returns an array of element IDs (e.g. for the User menu block, which is implemented by system module: 'system__user-menu') — rules are the only ones that currently return more than one — derived from the given context (the $variables array from the preprocess function).
contextual_links_handler
Returns an array of contextual links derived from the given context (the $variables array from the preprocess function).

[EDIT] This is used to add the 'edit skin' link to contextual menus for each block. The reason we used a callback here instead of relying on hook_contextual_links_view_alter() is due to modules like panels having their own slightly wonky (custom) version of contextual menus for panel panes.

by @sun
I'd have to have a closer look at the current code. However, it would be ideal if we could write down something along the lines of #16 in the new issue - but with some more essential details; for example, (not having looked at the code yet) the purpose of contextual_links_handler is not clear to me - I don't understand why some other module has to deal or assist with contextual links for Skinr UI at all. In such cases, we should also clarify the difference to having modules simply implement hook_contextual_links_view_alter() instead, or why that wouldn't be feasible.

The goal should be to have a single hook_skinr_config_info() [or whatever it was/is named] with clear and concise info properties being returned, and most of the module integration information being cached or cacheable. If something is based on dynamic values (retrieval/assignment), then there should be a dedicated hook that is being invoked, getting clear function arguments (for example, passing $form and $form_state to a preprocess_hook_callback is... highly confusing for me). Ideally, there should be default implementations provided by Skinr itself for the 80% use-case, so most module integration code doesn't have to implement common logic.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new8.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attached patch cleanly reproduces #14 in a test.

Sorry, contains "commented out" tests for faster debugging, to be reverted in final patch.

sun’s picture

StatusFileSize
new9.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attached patch fixes this issue.

Yay for resolving a bunch of @todos and introducing an entire new stack of @todos at the same time! ;)

sun’s picture

StatusFileSize
new10.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed testSkinrImplements().

sun’s picture

StatusFileSize
new11.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch skinr.kill-init.19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sorry for the noise. Removed stale code and added some clarifying comments/todos.

moonray’s picture

StatusFileSize
new84.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_db21c248c1be.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attached patch gets rid of all the crazy skinr_handlers.

What we're left with is the following hooks:

hook_skinr_config()
now returns an array of supported element types (see #1200352: $type and $module are used interchangeably in Skinr's code; fix this for the difference between an element type and the implementing module). I'm not entirely sure this is the best way to do this since we only need that list of element types for skinr_ui_filters() in skinr_ui.admin.inc; we might not need this function at all anymore if we can find a better way to prove the list of types.
hook_skinr_theme_hooks() and hook_skinr_theme_hooks_alter()
Provides a list of all available theme hooks for a given element. Used in skinr_ui_form_alter() in skinr_ui.module to determine whether a theme hooks limitation applies to the given element.
hook_skinr_elements()
Returns an array of element ids. These element ids are the keys used to store and retrieve skinr configs for page elements. This is used in the skinr_preprocess() and also to automatically generate contextual links (panels module being the exception, as always).

Things that still need doing for this patch:

  1. Make modules/*.skinr.inc load; module_load_includes() in module_hook() used by module_invoke() to automatically load includes based on the hook invoked only supports loading from module's directory: DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . "/$name.$type";.

    EDIT 2: One possible solution to this problem seems to be forking module_invoke_all(), but that sounds rather awful.

  2. Review the usefulness of hook_skinr_config() (see above)
  3. Write better code docs for hook_skinr_elements()
  4. Probably move the remaining functions out of skinr.handlers.inc and into their respective *.skinr.inc files. We can then eliminate this file entirely.
  5. Write better on-page help text in skinr_help() for the pages 'admin/appearance/skinr/edit/%/%/%' and 'admin/appearance/skinr/rules/edit/%'

I would like some feedback on this patch in the meantime, though, since it's a pretty radical change.

EDIT: We might also need a hook_skinr_elements_alter() function?

moonray’s picture

StatusFileSize
new93.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_13d6bab80e2e.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Looks like I was using an old patch from sun as a base. Updated to use sun's last patch in #19.

moonray’s picture

Status:Needs review» Needs work

It appears I broke sun's fix in #19. Setting to needs work.

moonray’s picture

StatusFileSize
new93.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_a899d8ae3f71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

See comment #20 for the remaining to-dos.

  1. With this patch I've got sun's fix working (with some modification), by using a skinr_invoke_all() as a wrapper for module_invoke_all() to automatically load the includes on demand when invoking hooks we know exist in the include files. The only problem with this approach is that panels.skinr.inc include a non-skinr hook to load their custom implementation of contextual links: hook_panels_pane_content_alter(). And since this file is not auto-loaded for the given hook (we can't use the skinr_invoke_all() wrapper to do that in this case) we don't get our contextual links for panel panes.
  2. We can't move all the hook_skinr_api_2() functions into their respective modules/*.skinr.inc files because then module_implments() won't pick them up. Should we drop these in skinr.skinr.inc? Or should they go in skinr.module?
sun’s picture

Status:Needs work» Needs review

Thanks for the major clean-up, @moonray!

However, I think that we should commit #19 first, and do the major rewrite of the module integration hooks only afterwards, possibly in a separate issue. I'd have a couple of remarks/objections on the current patch. To me it looks like #19 is a first step towards resolving/removing the global unconditional auto-loading, highlighting where things are still wrong, and/but to resolve the remaining bits, we need to change the module integration API hooks entirely, like you attempted to do in #20 onwards. As far as I can see, the changes complement each other, so as soon as #19 is in, we can move on with #23?

moonray’s picture

StatusFileSize
new11.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_077228b11a02.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

#19 is still giving me a bunch of errors in the tests.

  1. skinr.handlers.inc is always required for Skinr UI to function. The functions skinr_access_handler(), skinr_index_handler(), skinr_data_handler(), skinr_submit_handler() are all used by skinr_ui_form_alter() which builds the widgets form.

Attached patch (based on #19) fixes that by loading skinr.handlers.inc when those functions are due to be called in skinr_handler(). When we get rid of the handlers, this will go with it.

We still have the problem with panels' include, as I pointed out in #23, point 1:

The only problem with this approach is that panels.skinr.inc includes a non-skinr hook to load their custom implementation of contextual links: hook_panels_pane_content_alter(). And since this file is not auto-loaded for the given hook we don't get our contextual links for panel panes.

coltrane’s picture

Status:Needs review» Needs work

#25 seems to work fine with my example skin but a bunch of tests are failing.

Also, I'm unclear what is meant with the statements about including files and caching:

+++ skinr.module
@@ -465,9 +447,14 @@ function skinr_implements() {
+          // When rebuilding the cache, we auto-load the include file for
+          // convenience. Calling code should invoke skinr_load_includes()
+          // on the extensions they want to act with.
+          // @todo Remove this.
+          include_once DRUPAL_ROOT . '/' . $file;

Are you saying this include_once should be removed and all includes go through skinr_load_includes()?

+++ skinr.module
@@ -549,15 +541,15 @@ function skinr_implements() {
+ * @todo These hook implementations should not be required to be loaded at all,
+ *   rewrite the code that is currently not using the cache.

What is this code that is not using the cache? skinr_get_config_info() is loading from cache.

sun’s picture

Status:Needs work» Reviewed & tested by the community

Those @todos are mine, and we need to resolve them. Ideally, in this issue, after committing this patch.

+++ skinr.module
@@ -1314,6 +1318,10 @@ function skinr_config_info_default() {
function skinr_handler($type, $op, $handler, &$a3, $a4 = NULL, $a5 = NULL, $a6 = NULL, $a7 = NULL) {
+  if (in_array($handler, array('skinr_access_handler', 'skinr_index_handler', 'skinr_data_handler', 'skinr_submit_handler'))) {
+    module_load_include('inc', 'skinr', 'skinr.handlers');
+  }

Would be lovely to squeeze in a @todo here prior to committing this patch.

moonray’s picture

Status:Reviewed & tested by the community» Needs work

This still leaves us with broken panels support, which is not very cool (see#23, point 1, which applies to the first patch as well, not just my patch in #23).

moonray’s picture

The only thing I can think of to fix that panels issue is to create a new skinr_panels.module.
@sun, @coltrane: do you think that's an acceptable compromise, or is there possibly a better option?

moonray’s picture

Status:Needs work» Needs review
StatusFileSize
new15.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_26cbc0bdeda8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Attached is a patch that makes everything work, but I'm not happy about the way it's done due to it including code specific for panels (why does it always have to do thing differently form the rest of Drupal?).

Please review, and hopefully suggest a better approach. Or is my observation in #29 the only 'good' option?

moonray’s picture

Status:Needs review» Needs work

I decided to break out the panels functionality into its own module to prevent polluting Skinr API with panels specific hacks (and they are hacks). The new skinr_panels module will be included with skinr and skinr_ui modules, and has its own tests.

Committing so we can proceed with the next step.

moonray’s picture

StatusFileSize
new37.37 KB

Uh, and the patch.

moonray’s picture

Status:Needs work» Fixed
StatusFileSize
new106.44 KB

I updated and tweaked the additional changes from #23 in a new patch.
Committed.

If you have some input on the above 2 patches which warrant additional code, feel free to re-open this ticket.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.