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

[Rob Janes]

Well, the patch file that reverts your changes on my changes is short 
and free of conflicts, so I am not inclined to fork.  I need that patch 
cause I don't want to get the browser warnings again about a mishmash of 
http/https content.  Firefox doesn't seem to mind but IE complains bitterly.

With tongue in cheek, perhaps you could file this and just cut and paste 
it into an email the next time the situation arises:

"Hi (replace with name) Rob, I reverted some of your changes re (insert 
problem here) system_path due to (insert reason here) consensus 
conflict.  have a look, any q email. thx, Benoit."

That way I know 1. my changes were an issue, and MOST OF ALL 2. i better 
be careful next time I refresh the code, cause my changes were actually 
related to a problem i tried to solve rather than a blithe hack at the 
code base.

i was unaware of an irc channel, nor of any discussion about xxx_PATH 
convention in any incarnation of wifidog-auth.  Thanks for the tip.

The autodetect for SYSTEM_PATH was incorrectly done in 
path_defines_base.php.  I found and fixed that middle of march, tested 
it against various combos of apache document_root and apache aliases, 
and checked in the code.  Perhaps it might be worthwhile revisiting the 
setting of BASE_URL_PATH, etc etc in light of this.

more misc below ...

-rob


Benoit Grégoire wrote:

>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.   
>  
>
i guess i'm the fourth, counting my checkin of path_defines_base.php 
middle of march.

>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.
>  
>

sorry.  ignorance is bliss, until you step on a crack.  i sat on the 
code changes for a couple of weeks or so until i was pushed to check in 
the code defensively.  in that time i could have posted and maybe gotten 
a response.  although, as one of the authors of path_defines_base i did 
feel as though it was an area in my domain.  point taken though.

So setting BASE_URL_PATH to SYSTEM_PATH might be a friendly change?  
Then I could get rid of the http/https crap with a friendly patch, 
commit and then not have to worry about code merges?  hmmm.

>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.
>  
>
hmmmm.  well, i got the mishmash of http/https content, so it's not 
completely fixed yet.

>3-You incorrectly changed the behaviour of a constant (BASE_URL_PATH is NOT  
>supposed to change the mode back to HTTP).
>  
>
i guess that's a documentation issue?  When I fixed up 
path_defines_base.php i added a lot of comments about conventions for 
the php constants DOCUMENT_ROOT, SYSTEM_PATH and WIFIDOG_ABS_FILE_PATH.  
Perhaps something authoritative in an explanitory way could be added to 
path_defines_url_content.php?  Oh - done, I see.

>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).
>  
>
yes, i did, thanks.  made up a patch file to revert just the system_path 
changes.

>  
>
>>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. 
>  
>
The change_password page can be displayed in http or https, it should 
not and does not matter.  What matters is the post action.  If one is 
concerned about security, the post action must be https.  That's when 
the userid and password go out in the net.  Until then it's just an 
empty form.  The problem I currently have is that change_password 
accepts the https post but does not redirect to clear the https.  The 
reason it doesn't is it has to display the info message about the 
password change, and to get it to do that after a redirect is some extra 
fiddling I didn't have time for.

>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). 
>  
>
it would be nice if i got an email when a bug was assigned to me.  i had 
no idea.  I made that change cause of errors showing up in the log.

>  
>
>>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.   
>  
>
sounds good to me.

>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
> *   */
>  
>
good stuff, that's the docs I didn't see!

>*********************************************************************
>
>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.
>  
>
never done that before.  now that i know about it, i'll give it a try.

>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).
>  
>
thanks much.

>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.
>  
>
ummm, please see the email snippet up top.  In contrast, when I fix bugs 
in wifidog-auth I usually don't know whose bug I'm fixing (unless it's 
mine).

>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,
>
>  
>
good 'nuf!