Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anarcat’s picture

Status: Active » Needs work

Why do we need cookie_domain?

Please mark patches as "needs review" otherwise they are likely to be lost.

omega8cc’s picture

Status: Needs work » Needs review

It is required by some commonly used modules (like wysiwyg) and it must be in the the settings.php directly to work, while Aegir rewrites it.

Steven Jones’s picture

Status: Needs review » Needs work
+++ b/platform/provision_drupal_settings.tpl.php
@@ -77,6 +77,7 @@
+    $cookie_domain = $_SERVER['HTTP_HOST'];

You're just setting the $cookie_domain variable, before all the hard work to sanitise the $_SERVER['HTTP_HOST'] variable is undertaken.

I think that it should be moved to within the if statement below, so the two cookie domain properties are set together.

Powered by Dreditor.

omega8cc’s picture

$cookie_domain should be just $_SERVER['HTTP_HOST'], without the dot at the beginning, so there is no reason to move it, IMO. It is not the same as session.cookie_domain. The $cookie_domain defined that way is intended as a workaround for some popular modules requiring it.

Steven Jones’s picture

Looking through conf_init() in Drupal 6 it would seem that the $cookie_domain is set by Drupal, if it hasn't already been set, and that the session.cookie_domain is set to the value of $cookie_domain anyway.
So it's possible that Aegir doesn't need to do something that it is? Or that the modules that are using $cookie_domain aren't using it correctly?

I'm still not convinced that this is something that Aegir needs to be setting for all sites by default.

omega8cc’s picture

It is safe to have it defined for all sites and it resolves the issue with some popular modules, which require this set in settings.php or they will refuse to work properly with red warning on the site status page. And because it doesn't work when defined in the included files, it must be directly in the settings.php template to get it to work. Maybe all those contrib modules are wrong, but that is how it works.

omega8cc’s picture

I think it could be done also that way: http://drupal.org/node/984396#comment-3853716

Steven Jones’s picture

Drupal core will define it for you if it's not specified for the settings.php.

Can you provide links to the issues in the contrib projects that have an issue with the $cookie_domain variable please.

omega8cc’s picture

This has been discussed (and confirmed) many times on the IRC #aegir at freenode and we have it already fixed after more than 100 our clients requests, so it is fully confirmed, that defining it as explained *is* required, so I never even looked for this issue in the queues.

omega8cc’s picture

I believe some modules simply require using this, and it *is* defined as one of optional values in the Drupal vanilla default.settings.php, so it is not true that Drupal will always do that for you automatically. Of course the comment in default.settings.php refers to situation when you have multiple domains pointing to the same site, so it sounds irrelevant, but still, for some reason, some modules, like wysiwyg file managers, just require it.

Steven Jones’s picture

conf_init() in Drupal 6 will set the $cookie_domain variable if its not already been set:

http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/conf_i...

I am happy for a fix to go into Aegir, just not the current one, and would rather that Aegir didn't have to attempt to fix other, buggy modules. (What gets into Aegir or not is my decision btw.)

omega8cc’s picture

omega8cc’s picture

And here is some explanation, however I don't know if that is correct: http://drupal.org/node/719238#comment-2651560

Steven Jones’s picture

From looking at the CKEditor code, I would strongly suggest that this 'warning' by CKEditor module is not needed, but if you say that CKEditor doesn't work without setting it (actually doesn't work, not just saying it doesn't) then I guess there is something more to this.

anarcat’s picture

Status: Needs work » Closed (won't fix)

I agree with Steven here: we shouldn't try to fix broken modules. I have always been amazed by this bug, since there are really weird things here. First, I have never had those issues on our sites in aegir. Maybe there's a warning, but I wasn't told of any issues requiring complete changes to all settings.php files on the server (trust me, I would have heard of this). Second, why wouldn't a change in global.inc work? It's virtually the same file.

As for the reason why conf_init() doesn't work for that module, it seems that #719238: $cookie_domain not defaulted properly on CKFinder boostrap correctly explains it, more specifically:

Because CKFinder is launched "outside" Drupal engine, this setting is required to be able to bootstrap Drupal and access Drupal environment and sessions to check user permissions.

So it seems CKFinder works outside the scope of the regular Drupal bootstrap. However, I do believe this is a bug in the module: nowhere in Drupal's documentation or settings.php is it said that this line must be explicitely defined. This also makes it completely painful to do migrations and things like that. If all that is needed is to do:

+    $cookie_domain = $_SERVER['HTTP_HOST'];

... as proposed by the patch, why doesn't the module do that itself?

The reasoning behind $cookie_domain is to *override* that default value. A third-party module should set its own proper defaults if it bypasses Drupal's bootstrapping system.

I therefore would not fix this in Aegir.