Exclude edge cases where get_relative_url depends on CWD#2296
Conversation
When only one of the two passed paths starts with a slash, this function would produce results that depend on (and expose parts of) the actual current working directory.
In a similar manner, it was also possible to "break out" of the "current directory" by starting one of the paths with `../../..` etc.
Additionally, the fact that paths always end up being resolved relative to the current working directory made this function less efficient than it needs to be.
Fun fact: `get_relative_url('path_a', 'path_b')` actually ended up looking up how to get to `/current/working/directory/path_a` from `/current/working/directory/path_b`.
Luckily, none of these behaviors can ever actually come into effect (the function is always called without a leading slash and without paths deliberately trying to escape), but the fix is still good to reduce surprise.
*The actual fix* is that we make the leading slash ignored (or quite the opposite - we always add it).
Now when either of the two arguments try to go up higher than the top level, they just end up at the top level (e.g. `foo/../..` is effectively `.`), only then the "relative" calculation happens.
|
If your thinking is that this might get accepted more quickly than your other PR, you're wrong. My assumption is that the standard lib to correct in its handling of any input and performance is not enough of a motivator to get me to spend any time on it. And well, I just don't have the time to go through every scenario in your tests and confirm that every one is correct. Sorry, but I'm not willing to take your assertions as face value. I need to be convinced. To make matters worse, you obscure some of the tests with this: for url_slash in ('', '/'):
for other_slash in ('', '/'):Spell out every combination in text so I can see them all. Then, point out the ones which fail before the change (side-by-side tests showing which pass/fail for each implementation would be ideal). That will give me a lot less to focus on. |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
|
So at this point you have more than demonstrated that an issue exists. But now I need to go through every single one of those tests and confirm that what your "fix" is outputting is correct. That is going to take some time. Please be patient. |
|
Finally got around to reviewing this. Sorry for the delay and thanks for the work. |
2 participants
When only one of the two passed paths starts with a slash, this function would produce results that depend on (and expose parts of) the actual current working directory.
In a similar manner, it was also possible to "break out" of the "current directory" by starting one of the paths with
../../..etc.Additionally, the fact that paths always end up being resolved relative to the current working directory made this function less efficient than it needs to be.
Fun fact:
get_relative_url('path_a', 'path_b')actually ended up looking up how to get to/current/working/directory/path_afrom/current/working/directory/path_b.Luckily, none of these behaviors can ever actually come into effect (the function is always called without a leading slash and without paths deliberately trying to escape), but the fix is still good to reduce surprise.
The actual fix is that we make the leading slash ignored (or quite the opposite - we always add it).
Now when either of the two arguments try to go up higher than the top level, they just end up at the top level (e.g.
foo/../..is effectively.), only then the "relative" calculation happens.This pull request makes the behavior 100% the same as in #2272, but without the disadvantage of being a "custom replacement for the standard lib function". It also brings 35% of the performance advantage.
Build times of a site that has ~350 pages:
/home/oprypin/repos/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/athena-website:/home/oprypin/repos/athena-website:(meanwhile, #2272 achieves 6.65 sec)