Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
cookie_domain.patch | 1.02 KB | omega8cc |
Comment | File | Size | Author |
---|---|---|---|
cookie_domain.patch | 1.02 KB | omega8cc |
Comments
Comment #1
anarcat CreditAttribution: anarcat commentedWhy do we need cookie_domain?
Please mark patches as "needs review" otherwise they are likely to be lost.
Comment #2
omega8cc CreditAttribution: omega8cc commentedIt 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.
Comment #3
Steven Jones CreditAttribution: Steven Jones commentedYou'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.
Comment #4
omega8cc CreditAttribution: omega8cc commented$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.
Comment #5
Steven Jones CreditAttribution: Steven Jones commentedLooking 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.
Comment #6
omega8cc CreditAttribution: omega8cc commentedIt 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.
Comment #7
omega8cc CreditAttribution: omega8cc commentedI think it could be done also that way: http://drupal.org/node/984396#comment-3853716
Comment #8
Steven Jones CreditAttribution: Steven Jones commentedDrupal 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.
Comment #9
omega8cc CreditAttribution: omega8cc commentedThis 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.
Comment #10
omega8cc CreditAttribution: omega8cc commentedI 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.
Comment #11
Steven Jones CreditAttribution: Steven Jones commentedconf_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.)
Comment #12
omega8cc CreditAttribution: omega8cc commentedSee also:
http://drupal.org/node/719238
http://drupal.org/node/734580
Comment #13
omega8cc CreditAttribution: omega8cc commentedAnd here is some explanation, however I don't know if that is correct: http://drupal.org/node/719238#comment-2651560
Comment #14
Steven Jones CreditAttribution: Steven Jones commentedFrom 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.
Comment #15
anarcat CreditAttribution: anarcat commentedI 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:
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:
... 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.