Clearing cache on Skin settings page causes errors default groups from skinr_skinr_group_info() to not get loaded

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

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.

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.

EDIT: removed double post.

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.

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.

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.

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.

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?

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

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

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.
4. 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?

#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.

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).

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?

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?

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.

Uh, and the patch.

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.