Opened 9 years ago

Closed 8 years ago

#914 closed defect (fixed)

[patch] Admin js option does not honor absolute urls

Reported by: eugene@… Owned by: adrian
Component: contrib.admin Version: master
Severity: normal Keywords: patch
Cc: gandalf@…, nesh@…, mhf@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Previously js option produced script blocks using verbatim path. Example:

class Document(meta.Model):
    # fields
    class META:
        admin = meta.Admin(
            js = (
                '/tinymce/jscripts/tiny_mce/tiny_mce_gzip.php',
            ),
        )

produced:

<script type="text/javascript" src="/tinymce/jscripts/tiny_mce/tiny_mce_gzip.php"></script>

New admin produces different string:

<script type="text/javascript" src="/media//tinymce/jscripts/tiny_mce/tiny_mce_gzip.php"></script>

Essentially it is the same js path but it is now prefixed by /media/. It is easy to correct in my case: change symlink, and remove leading '/' in js.

Anyway we have a backward-incompatible difference. We have to decide how to handle it: old way or new way. The final decision should be documented.

New way is not bad. You can argue that it reduces number of entries in .htaccess or whatever non-Django mechanism is used for file serving. But it forces to change existing model files, which use extra JavaScript for Admin interface.

Attachments (4)

admin_modify.py.patch (912 bytes) - added by gandalf@… 9 years ago.
add checking for / and url's for js in admin media
admin_modify-mr.py.patch (888 bytes) - added by gandalf@… 9 years ago.
updated patch for magic-removal
admin_modify.diff (1.4 KB) - added by nesh <nesh [at] studioquattro [dot] co [dot] yu> 9 years ago.
diff against [3411]
absolute_include_admin_script-r4185.diff (1.1 KB) - added by mhf@… 8 years ago.
Patch (against r4185), which uses urljoin

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by edgars@…

+1

comment:2 Changed 9 years ago by Joeboy

I think the new behaviour is really quite irritating. If I want to use a single js file that varies from site to site (eg. a tinymce config file) I have to make a copy of the whole admin media folder for each site, whereas previously I could just stick it in /media. Am I missing something or is this just gratuitously annoying?

I'd have thought the sensible system would be to not prefix if the url begins with a slash.

comment:3 Changed 9 years ago by gandalf@…

  • Cc gandalf@… added
  • Keywords patch added
  • Version set to SVN

Here is a patch that modifies this behaviour in two ways:

a) It checks for / at the beginning of the lines, and if it finds it, it doesn't include ADMIN_MEDIA_URL before it
b) It checks with regular expression if it is url (starting with http, ftp or https) and also doesn't include ADMIN_MEDIA_URL in this case.

otherwise, it inserts ADMIN_MEDIA_URL at the beginning

Changed 9 years ago by gandalf@…

add checking for / and url's for js in admin media

Changed 9 years ago by gandalf@…

updated patch for magic-removal

comment:4 Changed 9 years ago by gandalf@…

  • Summary changed from Behavior of META js option to [patch] Behavior of META js option
  • Version changed from SVN to magic-removal

comment:5 Changed 9 years ago by gandalf@…

Any comment on why this patch is not acceptable for inclusion into svn?

comment:6 Changed 9 years ago by xale

  • priority changed from normal to high

It's been six months since this ticked was created and the patch still isn't in trunk as far as I can see. Is there any readon why it's not commited yet?

comment:7 Changed 9 years ago by xale

I've looked at the patch and it's pretty horible.

What we really need here is either remove this prefix at all and change all admin code to use full path or change templates and template tags to include user defined JS URLs without prefix.

comment:8 Changed 9 years ago by nesh <nesh [at] studioquattro [dot] co [dot] yu>

  • Cc nesh@… added

What is the status of this patch? I really need this because I don't put admin media within my media folder and I hate to use patched django source.

I'm definitely against prepending ADMIN_MEDIA_PREFIX to custom JS.

IMHO this patch is OK.

Just a suggestion, remove regexp from the function like this (small optimization).

_URL_PATTERN = re.compile(r'''(?x)((http|https|ftp)://(\w+[:.]?){2,}(/?|[^ \n\r"']+[\w/!?.=#])(?=[\s\.,>)"'\]]))''')
def include_admin_script(script_path):
   if _URL_PATTERN.match(script_path) or script_path[0] == '/':
       return '<script type="text/javascript" src="%s"></script>' % (script_path)
   else:
       return '<script type="text/javascript" src="%s%s"></script>' % (settings.ADMIN_MEDIA_PREFIX, script_path)
include_admin_script = register.simple_tag(include_admin_script)

Changed 9 years ago by nesh <nesh [at] studioquattro [dot] co [dot] yu>

diff against [3411]

comment:9 Changed 8 years ago by mhf@…

  • Cc mhf@… added

This problem can be solved with urlparse.urljoin:

>>> from urlparse import urljoin
>>> urljoin('/media', 'somescript.js')
'/somescript.js'
>>> urljoin('/media', '/anotherpath/somescript.js')
'/anotherpath/somescript.js'
>>> urljoin('/media', 'http://example.org/somescript.js')
'http://example.org/somescript.js'

Changed 8 years ago by mhf@…

Patch (against r4185), which uses urljoin

comment:10 Changed 8 years ago by mhf@…

  • Summary changed from [patch] Behavior of META js option to [patch] Admin js option does not honor absolute urls
  • Version changed from magic-removal to SVN

Attached a patch against r4185, which uses urljoin instead of a regular expression.

Should there be a regression test for this?

comment:11 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Ready for checkin

Thanks mhf, I'll let the core decide if we need a regression test and mark as ready for the moment.

comment:12 Changed 8 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

Marking this as fixed because, as of [4382] in the newforms-admin branch, you can implement the javascript() method on your class Admin to specify arbitrary JavaScript.

Note: See TracTickets for help on using tickets.
Back to Top