[isf-wifidog] Re: system_path - page content with mishmash of http
and https links
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,
> 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:
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
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.
Benoit Grégoire, http://benoitg.coeus.ca/
More information about the WiFiDog