Redirect loop

I've seen this issue, haven't tried the patch though. Fixed it by dropping the context config from the db manually and modifying the Feature it was in by hand then clearing the cache by dropping those db tables manually.

!module_exists($module) && 'enable' == $state)

Can lead to a loop if the module really doesn't exist (on the file system) but is in the configuration - likely why we usually see this in remote sites.

Try this, module_enable() exits with FALSE as soon as one of the modules it needs to enable cannot be found on the file system, causing loops I believe.

Patch only redirects if at least one module in the enable list is enabled after module_enable() is called. It also processes modules one at a time, so a module missing dependencies, or not found on the file system won't impact context_module trying to enable other modules in this context.

The patch in #4 just fixed another redirect loop for me. I think enabling modules one at a time in a foreach loop is the way to go.

From https://api.drupal.org/api/drupal/includes!module.inc/function/module_en...

Return value

FALSE if one or more dependencies are missing, TRUE otherwise.

My thinking was:

Say we have 2 modules, A and B. B has a dependency on C.

If we don't enable modules one at a time, using the logic that sets $redirect as in the patch, if A has no missing dependency and can be enabled safely but C is missing and so B can't be enabled safely the behaviour we want is:

- A is enabled, $redirect is set to TRUE
- B is given the default Drupal Core handling for modules with missing dependencies, $redirect is FALSE
- Throw a warning and/or watchdog that something went wrong in the process of handling B

If we pass module_enable() both A and B at the same time then $redirect will never be set to TRUE, so because of B's missing dependency, A is handled incorrectly and we skip the step of redirecting and clearing the cache.

This means we want to process our modules one-at-a-time to ensure we don't skip over carefully handling "safe" modules because of the existence of "unsafe" ones.

Does that make sense?

It's also possible that two conflicting contexts both reacting, one that disables and one that enables the same module would also result in a redirect loop.

This one could probably be dealt with by having the reaction build a list of modules to be enabled/disabled and then after all reactions have modified the list then having the modules enabled/disabled and drupal_goto() on hook_exit().

Yeah, I had considered that. I think we could justify splitting this thread into at least two separate issues.

- Take a 'Redirect' module approach to redirect loops, and detect and break the loop. Some sort of 'Redirect loop detected, Context Module reaction disabled because of [BLAH], please fix and click this button to re-enable'

Yeah cool, I don't think we have to detect the loop directly. It would be easier to just "detect" the FALSE returned by module_enable() and display a prominent warning at that point.

From my experience, what actually happens to module B is not a loop, but the module is enabled but is broken somehow (due to missing deps) and if you go to the modules page you see "missing" in red for the module dependencies. Downloading the missing deps and refreshing the modules page was enough to fix the issue, from memory.

The worst problem with unbroken loops is that it is actually difficult to fix them because you can't modify the context easily any more :( The only solutions to the loops I've found is to revert the context to a known working version (maybe lose a bunch of recent work) or apply the patch in #4 then dl the missing dependency.

It seems wrong that modules need to implement this "avoid loop" behaviour for drupal_goto() on an ad-hoc basis. I was going to open an issue against core but I see drupal_goto() has been removed in D8 https://drupal.org/node/2023537 and I don't have enough experience with RedirectResponse() to know if we have better tools to handle loops there.

For now I'll just say that avoiding loops by trying to detect them "on the fly" and then "breaking the loop" is tough for anonymous users who won't have sessions, or heavily cached sites that are not invoking hooks at all on cached pages.

Where are we at with this?

We had some general conceptual discussion, but I'm unclear on what that translates to in terms of code changes in the patch.

What's the plan to get this to the point where we can commit something?