#18826 closed Cleanup/optimization (worksforme)
A bit changed JavaScript for CSRF with async JS — at Version 5
Reported by: | panco | Owned by: | nobody |
---|---|---|---|
Component: | Documentation | Version: | 1.4 |
Severity: | Normal | Keywords: | ajax, csrf |
Cc: | lrekucki@… | Triage Stage: | Design decision needed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Recently I've found use for the code found at https://docs.djangoproject.com/en/dev/ref/contrib/csrf/#ajax
and as I can't sleep well if my JS doesn't pass some of the basic JSlint standards I've changed the snippet a bit:
jQuery(document).ajaxSend(function (event, xhr, settings) { function getCookie(name) { var cookieValue = null, cookies = [], i = 0, j = 0, cookie = {}; if (document.cookie && document.cookie !== '') { cookies = document.cookie.split(';'); for (j = cookies.length; i < j; i += 1) { cookie = jQuery.trim(cookies[i]); // Does this cookie string begin with the name we want? if (cookie.substring(0, name.length + 1) === (name + '=')) { cookieValue = decodeURIComponent(cookie.substring(name.length + 1)); break; } } } return cookieValue; } function sameOrigin(url) { // url could be relative or scheme relative or absolute var host = document.location.host, // host + port protocol = document.location.protocol, sr_origin = '//' + host, origin = protocol + sr_origin; // Allow absolute or scheme relative URLs to same origin return (url === origin || url.slice(0, origin.length + 1) === origin + '/') || (url === sr_origin || url.slice(0, sr_origin.length + 1) === sr_origin + '/') || // or any other URL that isn't scheme relative or absolute i.e relative. !(/^(\/\/|http:|https:).*/.test(url)); } function safeMethod(method) { return (/^(GET|HEAD|OPTIONS|TRACE)$/.test(method)); } if (!safeMethod(settings.type) && sameOrigin(settings.url)) { xhr.setRequestHeader("X-CSRFToken", getCookie('csrftoken')); } });
- "===" instead of "=="
- all vars at the beginning of the function (and all the changes that brings forth)
I realize these changes are very small, but I think since this is a simple copy/paste snippet it should be of the highest quality possible (there's room for improvement still).
I'm using it and it performs as intended.
Change History (5)
comment:1 by , 12 years ago
Has patch: | unset |
---|
comment:2 by , 12 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
The "Coding style" section in the docs is certainly missing a JavaScript section (this might be a good django-dev material). But until we get a one, I don't think making changes that hurt readability to satisfy this particular linter is a good idea.
comment:3 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
The changes described in the report (===
and proper variable declaration) usually don't make the code less readable and they're widely accepted good practices.
But there are other changes in the snippet I'm less comfortable with:
- using
ajaxSend
instead ofajaxSetup
— no idea, I'm not a frontend dev; - re-compiling
getCookie
,sameOrigin
and `safeMethod' every time — looks wasteful.
If there was a proper patch against the documentation, I would be willing to reconsider this ticket.
comment:4 by , 12 years ago
Triage Stage: | Accepted → Design decision needed |
---|
comment:5 by , 12 years ago
Cc: | added |
---|---|
Description: | modified (diff) |
I mainly meant things like:
- 8 spaces of indentation;
- very long line ending with a comment;
- initializing
cookie
to an object while it's later assigned a string; - not initializing value of
i
in the loop while initializing aj
with list length for a premature optimization.
Changing to ===
is ok, while hoisting all the variables to the top, IMHO, actually makes the code less readable (but it's probably a matter of taste).
My feeling is that the current code is functional and works fine, but I'll defer if another core dev wants to accept this.