https://www.chiliproject.org/2011-03-13T02:48:04+01:00ChiliProjectChiliProject - Bug #273: custom autologin cookie name not readhttps://www.chiliproject.org/issues/273?journal_id=99742011-03-13T02:48:04+01:00Ted Pted.pham@gmx.com
<ul><li><strong>Start date</strong> set to <i>2011-03-13</i></li><li><strong>Estimated time</strong> deleted ()</li><li><strong>Priority</strong> changed from <i>Normal</i> to <i>Normal</i></li><li><strong>Project</strong> set to <i>ChiliProject</i></li><li><strong>Target version</strong> set to <i>1.2.0</i></li><li><strong>Assignee</strong> set to <i>Eric Davis</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>0</i></li><li><strong>Subject</strong> set to <i>custom autologin cookie name not read</i></li><li><strong>Category</strong> set to <i>User accounts</i></li><li><strong>Tracker</strong> set to <i>Bug</i></li><li><strong>Due date</strong> deleted ()</li><li><strong>Subproject of</strong> deleted ()</li><li><strong>Description</strong> set to <i>When you configure the autologin cookie name in configuration.yml, it is stor...</i> <a href="/journals/9974/diff/description" class="lightbox-ajax">More</a></li><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul> ChiliProject - Bug #273: custom autologin cookie name not readhttps://www.chiliproject.org/issues/273?journal_id=14892011-03-20T08:35:32+01:00Felix Schäferfelix+chili@oh14.de
<ul><li><strong>Target version</strong> set to <i>1.2.0</i></li><li><strong>Assignee</strong> set to <i>Felix Schäfer</i></li></ul><p>Ted P wrote:</p>
<blockquote>
<p>Possibly, the cookie name could be set globally somewhere.</p>
</blockquote>
<p>There is one in <code>Redmine::Configuration</code>, though I'm not really sure I like it. Anyway, I'm going to take a look at that.</p> ChiliProject - Bug #273: custom autologin cookie name not readhttps://www.chiliproject.org/issues/273?journal_id=14912011-03-20T09:46:22+01:00Felix Schäferfelix+chili@oh14.de
<ul></ul><p>If I didn't know better I'd say <code>Redmine::Configuration</code> is a bad joke <code>-_-</code>.</p>
<p>Anyway, I've updated your patch and made a pull request with it at <a class="external" href="https://github.com/chiliproject/chiliproject/pull/30">https://github.com/chiliproject/chiliproject/pull/30</a>.</p>
<p>Now, this had me working with <code>Redmine::Configuration</code>, which strikes me as half-assed at best. While having a central configuration always reminds me of the often-loathed windows registry, I can also see that it has advantages.</p>
<p>What's happening with <code>Redmine::Configuration</code> though apparently is that every place that reads a value from there also has to define a default in-place (<code>cookie_name = Redmine::Configuration['autologin_cookie_name'] || 'autologin'</code>), meaning if you every want to change that default, you'd have to go through everywhere this configuration piece is used and change it. Not nice.</p>
<p>You'd think there's a place to put defaults for those, you open the corresponding file and oh joy, you stumble over a hash named <code>@defaults</code> (source:/lib/redmine/configuration.rb#L21) with the comment "Configuration default values". Looks exactly like what you'd need, right? Well, nearly, the problem is that every key in source:/config/configuration.yml.example is in there uncommented, meaning it gets read with a value if <code>nil</code>, which in turn overwrites the so-called "defaults" from <code>@defaults</code>. To make matters worse, the "base" or "common" section in the source:/config/configuration.yml.example that gets loaded before any environment-specific configuration is called <code>default</code> too, and that gets patched together programmatically in <code>Redmine::Configuration</code>, instead of leveraging <a href="http://en.wikipedia.org/wiki/YAML#Data_merge_and_references" class="external">similar functionality provided by YAML</a> directly (though I don't know whether the YAML-one does a flat or a deep merge…).</p>
Anyway, all this ranting for this:
<ul>
<li>Should we consider key with a <code>nil</code> value in the configuration file as unset?</li>
<li>Should we keep it as-is and uncomment the keys in source:/config/configuration.yml.example so that the <code>@defaults</code> in source:/lib/redmine/configuration.rb#L21 really is a default config?</li>
<li>Should we keep the defaults at the place where the variable gets used?</li>
</ul>
And on a more philosophical/semantic matter:
<ul>
<li>Can we rename the "default" section in the config file to "common"?</li>
<li>Should we switch the config file to using yaml references instead of merging it in in source:/lib/redmine/configuration.rb?</li>
</ul> ChiliProject - Bug #273: custom autologin cookie name not readhttps://www.chiliproject.org/issues/273?journal_id=14922011-03-20T10:05:18+01:00Felix Schäferfelix+chili@oh14.de
<ul><li><strong>Assignee</strong> deleted (<strike><i>Felix Schäfer</i></strike>)</li><li><strong>Status</strong> changed from <i>Open</i> to <i>Ready for review</i></li></ul> ChiliProject - Bug #273: custom autologin cookie name not readhttps://www.chiliproject.org/issues/273?journal_id=15142011-03-22T23:42:09+01:00Eric Davis
<ul></ul><p>Felix Schäfer wrote:</p>
<blockquote>
<p>If I didn't know better I'd say <code>Redmine::Configuration</code> is a bad joke <code>-_-</code>.</p>
<p>Anyway, I've updated your patch and made a pull request with it at <a class="external" href="https://github.com/chiliproject/chiliproject/pull/30">https://github.com/chiliproject/chiliproject/pull/30</a>.</p>
</blockquote>
<p>Pull request looks good to me as long as the tests are passing</p>
<blockquote>
<p>Now, this had me working with <code>Redmine::Configuration</code>, which strikes me as half-assed at best.</p>
</blockquote>
<p>We should discuss this in the forums. From what I saw Configuration included some of Setting which has it's own set of edge cases. I'm in favor of refactoring it if it improves the internal design.</p> ChiliProject - Bug #273: custom autologin cookie name not readhttps://www.chiliproject.org/issues/273?journal_id=15262011-03-24T21:26:44+01:00Eric Davis
<ul><li><strong>Assignee</strong> set to <i>Eric Davis</i></li><li><strong>Status</strong> changed from <i>Ready for review</i> to <i>Closed</i></li></ul><p>Felix:</p>
<p>I had a bunch of problems getting the tests to pass. Looks like the configuration.yml is used in the test environment and it's easy to get it messed up if you don't update your local version. +1 on working on Configuration some more...</p>
<p>Merged to master.</p>