Return redirection for symlinks #87
Conversation
| var deprecate = require('depd')('send') | ||
| var destroy = require('destroy') |
There was a problem hiding this comment.
Please undo all these changes.
There was a problem hiding this comment.
I just put the requires sorted alphabetically and grouped by build-ins, third-parties and local modules, as it's usually accepted in the Node.js community. Also I've made the requires style uniform, instead of some of them using its own var statement and others a common one, and some requires that were hidden between the variables declarations. Do you still want these clean-up changes to be undone?
There was a problem hiding this comment.
Do you still want these clean-up changes to be undone?
Yes. It is extremely important to us that git blame stays intact, thus why there is a mixed style.
|
Hi! Thanks for the pull request! Please undo all the style changes, mainly because they are unwelcome (we weren't asking for style changes) and it makes it really hard for me to understand what changed from looking at the diff. Also, please add documentation and tests for the new feature so we can accept it :) |
index.js
Outdated
|
|
||
| function responseStatus(res, code, msg) | ||
| { | ||
| if(msg == null) |
There was a problem hiding this comment.
Please match existing styles, for example the bracket style really stands out as misplaced.
|
Hi! Your original suggested I agreeded to was "Add an option to show the symbolic links as 301 redirections to the file location instead of return the file content.". I don't see an option to enable the feature. If this always enabled? If not, what is the option? We need an option to enable/disable this feature. If you want it accepted any time soon, you should probably default it to disabled so it's backwards compatible. |
|
Oh, sorry, looks like you added a test from when I looked at it hours ago, haha. Please try to add more tests, including non happy paths. A new feature should have many tests, not just one, as there are probably edge cases you need to test for, for example, what about folder junctions vs folder symlinks? I wouldn't expect a junction to redirect, is that the case with these changes or no? |
|
I've just done your suggestions :-) |
index.js
Outdated
| if (listenerCount(this, 'directory') !== 0) { | ||
| this.emit('directory') | ||
| return | ||
| } | ||
|
|
||
| if (this.hasTrailingSlash()) { | ||
| this.error(403) | ||
| return | ||
| return this.error(403) |
There was a problem hiding this comment.
This should be returning undefined, rather than the event emitter instance.
There was a problem hiding this comment.
I should have missed it before. Anyway it doesn't matter if it returns the event-emitter or undefined as far as it's not being used, and in fact I've seen in several places that this is the recomended way to do it. Anyway, I've already fixed it.
|
I've just set 403 error by default if destination is outside files root. |
|
Any update on this? |
|
Hi @piranna, sorry, I have not taken a look since you changed the pull request. Can you at least take a look and fix the failing tests so the pull request passes the test while I take a look at the new changes? Also note that one of the tests is marked as "double callback!" with your changes, so even though that's not causing a failure, you'll need to get that fixed as well. |
|
Tests are failling to some unrelated test I can't be able to identify what's the source of the problem ("should return 500 on error when reading file"). Could you be able to take a look so this could be merged? |
|
The problem that's giving is about the callback being called twice, probably due to the |
|
There are still a lot of failing tests and you haven't fixed the double callback, either. In addition, it looks like when you merged, you accidentally reverted code, including a security fix. Please fix all that up and ping me and I'll come back and take a look :)! |
|
I am going to close this. It seems like a good idea but we would need to have a much more clear and clean change set to ever merge this, which seems unlikely at this point. Feel free to reopen a new PR with just the minimal changes required to land the feature if you are willing and able to work on this again in the future. |
This fixes issue #86