Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WiP] Implement four-level CMS_PLACEHOLDER_CONF #5006

Closed
wants to merge 17 commits into from

Conversation

yakky
Copy link
Member

@yakky yakky commented Feb 14, 2016

Currently CMS_PLACEHOLDER_CONF only allows values for placeholder and template+placeholder keys
Sometimes a global configuration would make things much nicer / simpler
@ojii suggested a for level structure like (in increasing order of precedence)

CMS_PLACEHOLDER_CONF['*'] (global)
CMS_PLACEHOLDER_CONF['template'] (if template is given)
CMS_PLACEHOLDER_CONF['placeholder']
CMS_PLACEHOLDER_CONF['template placeholder'] (if template is given)

This is backward-compatible and allow a few neat tricks.

We can also add wildcards to placeholder configuration:

CMS_PLACEHOLDER_CONF['placeholder-*']

this will recognize placeholder-foo and placeholder-bar.
We need to make * (or %) reserved to avoid clashes (this will also solve the @ojii's comment regarding '*' being a valid placeholder name). We can use wildcards also for templates.

Changes needed

  • 4-level configuration
  • tests
  • documentation

On top of this is easy to implement plugin blacklisting (see #5000, #4979), list of plugins to show in text editor and other plugin configuration options

@yakky yakky added this to the 3.3 milestone Feb 14, 2016
@yakky yakky force-pushed the feature/global_placeholder_conf branch from e230637 to e3c9cf1 Compare February 14, 2016 18:55
@ojii
Copy link
Contributor

ojii commented Feb 15, 2016

Thinking about this a little more, I think the "global" tag should be None, not '*', as '*' is technically a legal placeholder name.

@yakky
Copy link
Member Author

yakky commented Feb 15, 2016

any comments @divio/django-cms-core ?

@carloratm
Copy link
Contributor

May be an idea to use regexp as CMS_PLACEHOLDER_CONF keys?

@yakky
Copy link
Member Author

yakky commented Mar 11, 2016

@ratmcarlo It may look weird, but it totally makes sense to me

@czpython
Copy link
Contributor

@yakky I like the idea, however I share agree with @ojii that * can be problematic.

@yakky
Copy link
Member Author

yakky commented Mar 15, 2016

Using regexp as suggested by carloratm can solve this as we can use None as default, and rely on regexp to handle wildcards

@yakky
Copy link
Member Author

yakky commented Mar 18, 2016

@czpython @ojii have a look at this proposal from @fmarco yakky#16
It uses general purpose regexp and it does not dictate any specific syntax or reserved keyword

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 88.462% when pulling d3acd9c on yakky:feature/global_placeholder_conf into 9c92ccb on divio:develop.

@yakky yakky force-pushed the feature/global_placeholder_conf branch from d3acd9c to a609c3a Compare April 6, 2016 08:05
@yakky
Copy link
Member Author

yakky commented Apr 6, 2016

@divio/django-cms-core can I have a feedback on the proposed implementation? It still has rough edges, but before proceeding I'd like a yay / nay if it's the way to go

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 88.474% when pulling a609c3a on yakky:feature/global_placeholder_conf into e753f72 on divio:develop.

@czpython
Copy link
Contributor

czpython commented Apr 7, 2016

I'm a bit skeptical about supporting regex, I'm not sure if this feature balances out with the extra technical implications.
I think the original idea (4 level conf) is good just not sure about regex.
If anything I'd say that regex in the template might be a bit too much.

@ojii
Copy link
Contributor

ojii commented Apr 7, 2016

I'm a bit skeptical about supporting regex, I'm not sure if this feature balances out with the extra technical implications.
I think the original idea (4 level conf) is good just not sure about regex.
If anything I'd say that regex in the template might be a bit too much.

I agree. regex seems like overkill and a solution looking for a problem to solve.

@yakky
Copy link
Member Author

yakky commented Apr 7, 2016

Regexp are actually a special case. You can easily use it without them and falling back to the "basic" 4 level structure
This allows to cover cases like "use a specific set of plugins in all the placeholders that starts with 'sidebar-'"
In simple websites this is an overkill, in reasonably sized projects it's almost essential. Especially if you do configure carefully the plugins in placeholders (I may share the regular hundred-lines long CMS_PLACEHOLDER_CONF we are forced to write, sometimes...)

@mkoistinen
Copy link
Contributor

The implementation looks great and while I support the effort here, having this much complexity here, in a setting, makes me wonder if we're going about this all wrong. I don't have any proposed solutions to change from CMS_PLACEHOLDER_CONF, but this seems to be expanding into a lot more than originally intended.

@yakky yakky force-pushed the feature/global_placeholder_conf branch from a609c3a to 3d704e2 Compare May 8, 2016 14:37
@yakky
Copy link
Member Author

yakky commented May 8, 2016

@divio/django-cms-core restructured to use simple key parsing, allowing to customize it

@coveralls
Copy link

coveralls commented May 8, 2016

Coverage Status

Coverage increased (+0.01%) to 88.474% when pulling a609c3a on yakky:feature/global_placeholder_conf into e753f72 on divio:develop.

@yakky yakky self-assigned this May 11, 2016
@yakky
Copy link
Member Author

yakky commented May 12, 2016

@mkoistinen @czpython could you please provide a comment on this last approach?

@mkoistinen mkoistinen modified the milestones: 3.3.0, 3.3.0.RC1 May 16, 2016
@mkoistinen mkoistinen modified the milestones: 3.3.1, 3.3.0 May 23, 2016
czpython and others added 17 commits June 8, 2016 14:00
Allow apphook endpoints to disable csrf checks
docs:intro:apphooks: "next" section wasn't Third-Party Apps
…projects

As per [their blog post of the 27th April](https://blog.readthedocs.com/securing-subdomains/) ‘Securing subdomains’:

> Starting today, Read the Docs will start hosting projects from subdomains on the domain readthedocs.io, instead of on readthedocs.org. This change addresses some security concerns around site cookies while hosting user generated data on the same domain as our dashboard.

Test Plan: Manually visited all the links I’ve modified.
* Changed default value for CMS_INTERNAL_IPS

* Fix typos
@yakky yakky force-pushed the feature/global_placeholder_conf branch from 3d704e2 to 8fdb958 Compare June 14, 2016 17:09
@yakky yakky closed this Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants