Smarter way of handling settings (fixes gunicorn/flup) #1565

Merged
merged 18 commits into from Jan 16, 2013

4 participants

@ojii

Removed monkeypatching from cms.init
Removed cms.conf.*
Added cms.utils.conf.get_setting(name)
Refactored all (CMS) setting accesses throughout the code base

This should fix issues with more recent gunicorn version (>0.12) as
well as make the code less hacky.

Known issues on this branch:

  • ApphooksPageLanguageUrlTestCase.test_page_language_url_for_apphook currently fails, not sure why.
  • SettingsTests.test_cms_templates_length, SettingsTests.test_cms_templates_none and SettingsTests.test_cms_templates_valid all fail currently because cms.conf.patch.post_patch and cms.conf.patch.post_patch_check are no longer available.
@ojii ojii Smarter way of handling settings
Removed monkeypatching from cms.__init__
Removed cms.conf.*
Added cms.utils.conf.get_setting(name)
Refactored all (CMS) setting accesses throughout the code base

This should fix issues with more recent gunicorn version (>0.12) as
well as make the code less hacky.

Known issues on this branch:

* ApphooksPageLanguageUrlTestCase.test_page_language_url_for_apphook
  currently fails, not sure why.
* SettingsTests.test_cms_templates_length,
  SettingsTests.test_cms_templates_none and
  SettingsTests.test_cms_templates_valid all fail currently because
  cms.conf.patch.post_patch and cms.conf.patch.post_patch_check are
  no longer available.
accaf20
@adaptivelogic

I would prefer something like:

from cms.utils import cms_settings
cms_settings.PERMISSION

rather than

from cms.utils import get_setting
get_setting('PERMISSION')

(Implement by making cms_settings an instance of a class defined in cms.utils.conf, with property getters for complex properties and a general getattr for everything else)

This is a stylistic preference, but would make the code more similar to regular django settings usage.

@ojii

apparently also affects FLUP/FCGI. Marking this as a blocker as not having this patch just breaks on too many deployment options.

@digi604
Divio AG member

I like the getter approach from Björn

@ojii

link?

@digi604
Divio AG member

Björn = adaptivlogic

@ojii ojii Merge branch 'develop' into dont-patch-configs
Conflicts:
	cms/admin/change_list.py
	cms/admin/pageadmin.py
	cms/api.py
	cms/models/pagemodel.py
	cms/signals.py
	cms/tests/admin.py
	cms/tests/page.py
64ff20a
@digi604 digi604 commented on the diff Jan 15, 2013
cms/conf/patch.py
- from django.template.loaders.app_directories import Loader
- except ImportError:
- return # south...
- for template in settings.CMS_TEMPLATES:
- if template[0] == settings.CMS_TEMPLATE_INHERITANCE_MAGIC:
- continue
- if not validate_template(template[0], ['js', 'css']):
- raise ImproperlyConfigured(
- "The 'js' and 'css' sekizai namespaces must be present in each template, "
- "- or a template it inherits from - defined in CMS_TEMPLATES. "
- "I can't find the namespaces in %r."
- % template[0]
- )
- VALID_LANG_PROPS = ['code', 'name', 'fallbacks', 'hide_untranslated', 'redirect_on_fallback', 'public']
-
- if isinstance(settings.CMS_LANGUAGES, tuple):
@digi604
Divio AG member
digi604 added a note Jan 15, 2013

Do we still have a check for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii

Should be pretty much done from my point of view besides one thing (which I really want as part of this pull request, since this is a blocker), which is a management command to check your settings. This would do something similar to what post/pre patch checks did before, but in a much more user friendly way. This will follow tomorrow morning (hopefully) my time (JST).

While working on this, I noticed that we really need more thorough testing of cms.utils.i18n (and cms.utils.conf...). Especially edge cases. That will be a different pull request though.

A few things to discuss while I'm asleep. Should someone have issues, I'll try to resolve them (or ignore them as I see appropriate). If you think there's something in this PR that really needs to be discussed and prevents a merge, please do say so in the comments here as I'll merge this once I have the command tomorrow.

A few things you might have an opinion on:

  • Do we keep get_setting internal or should this be documented?

  • In cms.utils.conf, is there too much magic somewhere? If so, how can we reduce it? (If you just say that "xyz is too magical" without a way to fix it, I'll ignore you, sorry.)

@digi604
Divio AG member

lgtm

@ojii

the last bit of this pull request will have to wait till later today, really want to make sure I get it right and didn't have time to finish this morning.

ojii added some commits Jan 16, 2013
@ojii ojii Installation checking infrastructure
* Added cms.utils.check.check
* Added 'cms check' command
* Added infrastructure to write installation checks
* Added first simple sekizai check
c72dfc5
@ojii ojii fixed runshell.py 875813e
@ojii ojii fixed reporting of cms.utils.check.FileSectionWrappers 963c261
@ojii ojii fixed old-style language setting transition 7b24457
@ojii ojii Added more checks to 'cms check'
* Added checks for deprecated settings CMS_MODERATOR and CMS_FLAT_URLS
* Added check for new-style language setting
* Added more checks for sekizai
3342a6e
@ojii ojii Finished cms check command
* Added docstrings to cms.utils.check module
* Added tests for cms.utils.check
* Fixed sekizai check function always reporting successful
bcd3073
@ojii ojii added docs for cms check and get_settings 9b4706c
@ojii

I'd consider this PR finished. Go nitpick about my code as I always do with your code ;-)

@digi604
Divio AG member

python 2.5 seams not to like it?

@ojii

and I don't like Python 2.5.... fix incoming.

@digi604
Divio AG member

Could you update the tutorial and the installation docs with this new command?

@digi604 digi604 merged commit a7ebce0 into divio:develop Jan 16, 2013
@evildmp

Commit accaf20 breaks some things. It could be their fault or something else's, will take a look:

Unhandled exception in thread started by <bound method Command.inner_run of <django.contrib.staticfiles.management.commands.runserver.Command object at 0x2c57b90>>
Traceback (most recent call last):
  File "/home/daniele/test-2013-jan-02/src/django/django/core/management/commands/runserver.py", line 91, in inner_run
    self.validate(display_num_errors=True)
  File "/home/daniele/test-2013-jan-02/src/django/django/core/management/base.py", line 266, in validate
    num_errors = get_validation_errors(s, app)
  File "/home/daniele/test-2013-jan-02/src/django/django/core/management/validation.py", line 30, in get_validation_errors
    for (app_name, error) in get_app_errors().items():
  File "/home/daniele/test-2013-jan-02/src/django/django/db/models/loading.py", line 158, in get_app_errors
    self._populate()
  File "/home/daniele/test-2013-jan-02/src/django/django/db/models/loading.py", line 64, in _populate
    self.load_app(app_name, True)
  File "/home/daniele/test-2013-jan-02/src/django/django/db/models/loading.py", line 88, in load_app
    models = import_module('.models', app_name)
  File "/home/daniele/test-2013-jan-02/src/django/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/home/daniele/test-2013-jan-02/src/semanticeditor/semanticeditor/models.py", line 6, in <module>
    template_list = [(f,n) for (f,n) in settings.CMS_TEMPLATES if f != settings.CMS_TEMPLATE_INHERITANCE_MAGIC]
  File "/home/daniele/test-2013-jan-02/src/django/django/utils/functional.py", line 185, in inner
    return func(self._wrapped, *args)
AttributeError: 'Settings' object has no attribute 'CMS_TEMPLATE_INHERITANCE_MAGIC'
@ojii
@evildmp

It used to be a setting though, and should be documented.

@ojii
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment