[isf-wifidog] Re: system_path - page content with mishmash of http and https links

Benoit Grégoire bock at step.polymtl.ca
Sam 29 Avr 16:59:14 EDT 2006


On April 29, 2006 12:24 pm, Rob Janes wrote:
> Hi Benoit-
>
> It would be polite to send an email about reverting a specific change I
> made.  Just because I'm a volunteer and my time is free doesn't mean I
> like to waste my time.
>
> The entry you made in CHANGELOG seemed to suggest that my change caused
> many errors for dev to resolve.  I did test that change against internet
> explorer and firefox, against the base wifidog code from trunk before I
> checked it in.  

You misunderstood.  The entry meant that the previous changes to path handling 
(the removal of BASEPATH at the top of every script and the autodetection of 
path values, the split in two abstraction levels (path_defines_base.php for 
code, path_defines_url_content.php for links and templates) took 3 developers 
weeks of discussion on IRC and coding to get back in a bug-free state, and 
most importantly to get it consistent across all the code.   

The reason I reverted your changes isn't that your solution was 
technically incorrect.  In fact, I think (didn't check) that we could just 
define BASE_URL_PATH to SYSTEM_PATH with no ill effects.  The reason I 
reverted PART of your changes is that it that:
1-Your patch basically reverted chunks of the cleanup work of three other 
developers AND changed a coding convention that was abundantly discussed on 
IRC without prior discussion.
2-It made the code inconsistent (there are over 99 references to BASE_URL_PATH 
in the code, you only changed about two dozen to SYSTEM_PATH).  If we are to 
ever reach 1.0, we have to have consistency in the way we do things to guide 
new developers.  Note that this doesn't mean we can't change them to better 
(or even just different) conventions.  For instance if you want to discuss 
using SYSTEM_PATH instead of redefining BASE_URL_PATH, I am fully open to 
discussing it.  However I don't want two "different but equal in every way" 
conventions.    No one thinks that the $%&"?/$?/$ PATH system is perfect, but 
it's now consistent, it works, and it took A LONG TIME to get this way.  
Anyone attempting a revamp it has to be willing to do it troughout the code.
3-You incorrectly changed the behaviour of a constant (BASE_URL_PATH is NOT  
supposed to change the mode back to HTTP).

I don't know if you reviewed the diff of my "revert".  If you do you'll see 
that I kept all the functional changes you made (save for a single error, 
see below).

> So, I am very interested to know what it was that I 
> broke, if anything.

Your switch from BASE_* to SYSTEM_PATH only had a single actual functional 
error, in  trunk/wifidog-auth/wifidog/templates/sites/index.tpl.  The link to 
change_password.php should switch the mode to SSL (if available), else both 
the old and new password will be transmitted in the clear.  For the rest I 
simply changed it to BASE_URL_PATH, keeping all your functional changes. 

On the other hand, your change in the definition of BASE_URL_PATH would have 
caused a bunch of links and images to revert to HTTP when that isn't 
necessarily intended.  Obviously I didn't check exactly where, there are 99 
references to it in the code.

Note that your commit also caused https://dev.wifidog.org/ticket/127, but that 
had nothing to do with the revert.  It's quite normal that you were mislead 
by legacy code about to be stripped off (that's why I'm trying to get rid of 
such old code, it causes new developers to make incorrect assumptions). 

> Now, the reason I made the change:
>
> All the links on the pages were full url links with the http://hostname/
> in front, or https.  The main problem was that I would get pages with a
> mixture of http and https links in them.  Everytime the page loaded I
> would get this prompt from the browser about this.  Browsers really
> don't like an https page with http content on it.  So I figured, this is
> really stupid.  The way it should be done is that all links on the page
> that don't go offsite should not have the http/https and hostname in
> front.  If you do that you are just asking for trouble.
>
> Putting base_url_path and base_ssl_path all over the place is just
> asking for trouble.  Unless otherwise required you should be using
> system_path, which is an absolute path, ie rooted at /, but without the
> http/https and hostname.  Full URL paths with http/s and hostname should
> be reserved for links that change the mode of the connection from secure
> to non-secure or the reverse.
>
> Is this http/s mixup what your dev had trouble with?  System path is the
> proper way to fix this.

I fully understand the (perfectly correct) reasoning for you patch.  It fixed 
actual problems (caused by incorrect usage of  BASE_SSL_PATH) that we also 
had but didn't get around to fixing.  

That being said I can't think of any case where SYSTEM_PATH would give a 
different result than BASE_URL_PATH.  I think you missed that there are 3 
constants, not two.  BASE_URL_PATH is NOT the opposite of BASE_SSL_PATH:  
BASE_NON_SSL_PATH is.   

I tried to clarify the documentation at the top of 
path_defines_url_content.php when I reverted, it now reads:
 
 /* This section deals with PATHs used in URLs and local content
 * BASE_SSL_PATH should be used to enter SSL mode (if available)
 * BASE_NON_SSL_PATH should be used to break out of SSL mode of when we
 * explicitly do not want someting to be referenced over http
 * BASE_URL_PATH should be used in all other cases to avoid needless SSL 
warning
 *   */

*********************************************************************

Now obviously I can't write changelog entries this long, nor send a private 
email that just took me an hour to write everytime I correct something.  I 
know everyone (including me) takes reverts a little personal.  However this 
whole issue (I didn't think there was one until I got your email) could have 
been avoided if you had dropped in the channel for a few minutes to reach a 
consensus before making an incomplete convention change.

Note that I didn't waste ANY of your time spent fixing bugs.  I took MY time 
to fix something should have been discussed beforehand (and fixed a few minor 
problems along the way) instead of demanding that you do it.  I felt that it 
was more respectfull of you and your time, an less likely to hurt your 
feelings.  Apparently I was mistaken (or misinterpreted).

Furthermore fixing your patch while keeping all your bug fix took me 10 
minutes as opposed to this email which took over an hour.  I doubt you would 
have made the changes for me with a email that was any shorter.  I really 
wish I had more time and energy to write more emails, documentation and 
distribute well deserved taps in the back to everyone.  Unfortunately I am 
also a human.

Hopefully this email won't get misinterpreted, I tried to get your phone 
number, but didn't get it in time.  If you want to discuss this further, 
don't hesitate to call me to avoid any further misunderstandings.

Good night,

-- 
Benoit Grégoire, http://benoitg.coeus.ca/


More information about the WiFiDog mailing list