New Handler doesn't work if not at URLconf root

[DrMeers]

Just discovered that including 'feincms.urls' at a non-blank point the URLconf (e.g. '/content/') stopped working in an upgrade from 1.4.2 to 1.6.2.

url(r'^content/', include('feincms.views.legacy.urls')), works correctly

Losing this functionality seems like a backwards development?

Also discussed here: https://groups.google.com/forum/?fromgroups#!topic/django-feincms/vFwfGkYdbQg but the suggested solution doesn't work (even after adding .as_view(), the handler doesn't seem to cope with not being at the URLconf root)

[DrMeers]

OK, the problem here is that the legacy handler (correctly) used the component of the URL as passed by the view dispatching mechanism, whereas the cbv one disregards it and tries to do everything using the request alone. The path passed into the view's args should be used instead of request.path; that way it will have any irrelevant path content removed and correctly match pages.

I imagine the change was made to try to encapsulate everything into the manager's for_request method, but I think we'll need to add another parameter to this. I'll try to get a patch committed for this shortly.

[DrMeers]

Initial work here: https://github.com/DrMeers/feincms/commits/321_fix / c601dc7

Adding pages/ prefix to URLconf broke 13 tests immediately. Adding fix reduced this to 10. Most of these are just naive tests which don't take the potential URL context into consideration.

However we do need to decide whether PageManager.page_for_path should be made URL-context-aware. I think it makes most sense for 'path' not to represent a full URL path but just the page-hierarchy component, so therefore tests involving this method will need to be updated accordingly.

Also, shouldn't the FeinCMS views/urls technically live in feincms.module.page, since they depend on Page? I guess they're OK where they are whilst nothing else wants to be there.

[DrMeers]

Any thoughts on this? I'm surprised it has been classified as a "minor bug" and hung around for this long; I've been involved in several projects which have had to resort to ugly workarounds. I'm happy to continue developing the solution, but would like some feedback regarding the above (and anything that may have changed in regards to in over the past nine months) before I proceed further.

[mjl]

It's most certainly not a minor bug, I can see that people want to mount the CMS somewhere else besides at the root. Having tests for that scenario is a good start.

I think I remember having removed some (it seemed) unnecessary trickery with PATH_INFO from the handler some time ago, so this may be partly my fault.

Conceptually, we probably need to pass in a "root mount point" parameter down with the request, since it will be needed whenever we generate absolute urls for pages and whatnot; but everything else should work on "relative" paths internally.

I think at least. Any thoughts?

[matthiask]

I'm sorry, that's obviously a misclassification. Thanks for the correction.

The current implementation works well when the whole WSGI app is mounted in a subfolder (example.com/djangosite/). PATH contains /djangosite/, PATH_INFO only /.

I cannot remember the reason why we aren't simply using standard URL patterns, though.

Regarding the views and feincms.module.page: Strictly speaking it would be correct to move the views there, and thereby reinforcing the point that FeinCMS can be used to build any CMSes. I'm not sure whether it's worth it, though. Why not?

[mjl]

We should probably support both, I can see the value of mounting the wsgi app under /cms and then wanting only /cms/mypages to be handled by the FeinCMS but have other Apps handle other urls. Also, people may want to mount the CMS multiple times (for backward compat links perhaps) under different root urls.

[mjl]

So to recap and to put that into a test case. I think this should work:

urlpatterns += ...
     include('foo', include('feincms.urls'))
     include('bar', include('feincms.urls'))
)

then a Page in hierarchy /one/two should appear as /foo/one/two AND /bar/one/two, AND return the correct prefix + path in response to get_absolute_url().

Things to look out for that come to mind are the caching for PAGE-FOR-URL and _cached_url in the Page model.

[mjl]

This does pretty much what we want, but it's more a quick hack than a real solution. We need to make sure we work with the without-prefix path for all internal data structures and the prefix doesn't seep into the page handler somewhere.

--- a/feincms/views/cbv/views.py
+++ b/feincms/views/cbv/views.py
@@ -22,6 +22,8 @@ class Handler(ContentView):

     def dispatch(self, request, *args, **kwargs):
         try:
+            if len(args):
+                request.path_info = args[0]
             return super(Handler, self).dispatch(request, *args, **kwargs)
         except Http404, e:
             if settings.FEINCMS_CMS_404_PAGE:

Looking back in history, that's actually pretty much what the old legacy view did. Does anybody remember why we work with request.path_info now?

v1.6 does:

best_match_for_path(path or request.path, raise404=True))

[DrMeers]

Does anybody remember why we work with request.path_info now?

You mean instead of request.path?

https://docs.djangoproject.com/en/dev/ref/request-response/#django.http.HttpRequest.path_info

[alexzaporozhets]

Hi, I got the same problem in 1.7.4

    url(r'^pages/', include('feincms.urls')),

[matthiask]

I ran a few tests with the following change and the commit referenced above, and things seem to work fine with FeinCMS@next in my fork:

diff --git a/example/urls.py b/example/urls.py
index a8fa04e..a106aca 100644
--- a/example/urls.py
+++ b/example/urls.py
@@ -11,5 +11,5 @@ urlpatterns = patterns('',
     url(r'^media/(?P<path>.*)$', 'django.views.static.serve',
         {'document_root': os.path.join(os.path.dirname(__file__), 'media/')}),
     url(r'', include('feincms.contrib.preview.urls')),
-    url(r'', include('feincms.urls'))
+    url(r'^bla/', include('feincms.urls'))
 ) + staticfiles_urlpatterns()

[matthiask]

I think this issue has been fixed by 9fb1d56. Thanks!