django-cms / django-cms

The easy-to-use and developer-friendly enterprise CMS powered by Django
http://www.django-cms.org
BSD 3-Clause "New" or "Revised" License
10.13k stars 3.09k forks source link

[BUG] Placeholder rendering fails loudly without PageContent or Model #7952

Closed Will-Hoey closed 1 month ago

Will-Hoey commented 2 months ago

Description

Non-root AppHook pages or Views that aren't tied to a model with placeholders will fail loudly if their parent template has any placeholders that are rendered with {% placeholder "name" %}

In CMS3 this works fine when inheriting and it just ignores the placeholders, but in CMS4 it breaks because it seemingly tries to render the placeholders. For example in our base:

{% block hero %}
    {% placeholder 'hero' %}
{% endblock hero %}
class SomeView(DetailView):
    template_name = "app/template.html"

    def get_object(self, queryset=None):
        return request.user    # Something here that doesn't define any placeholders

If we leave this block in our template.html it will attempt to render and break with NoneType should implement get_template because there's no PageContent or toolbar object.

Steps to reproduce

Steps to reproduce the behavior:

  1. Create an app with at least 1 non-root view. Make sure there's a {{ placeholder "name" }} in the template
  2. Add the app to a page
  3. Go to the sub-page link
  4. See error

Expected behaviour

In CMS3 this simply doesn't render the placeholder. The page can load as normal with only the available placeholders

Actual behaviour

In CMS4 this fails loudly with NoneType should implement get_template

Additional information (CMS/Python/Django versions)

Only requires django-cms > 4

Linked issue

https://github.com/django-cms/django-cms/issues/7712

Do you want to help fix this issue?

fsbraun commented 2 months ago

@Will-Hoey I've created a fix. You are invited to test run it (to ensure it works for your environment).

Will-Hoey commented 2 months ago

@fsbraun I'm in the midst of testing this, but I realise that we have cms-versioning installed as well which also seems to have a similar plugin renderer which is causing a similar issue https://github.com/django-cms/djangocms-versioning/blob/master/djangocms_versioning/plugin_rendering.py#L48

AttributeError: 'NoneType' object has no attribute '_meta' when it runs the rescan_placeholders_for_obj(current_obj)

I'm looking at decoupling versioning from the project for now so I can test this fix in isolation

edit: when versioning is removed this fix resolves the initial issue. The page can load as expected

fsbraun commented 2 months ago

Yes, well done. We need to change both. Maybe there is a way of reducing code repetition.

fsbraun commented 2 months ago

@Will-Hoey It turns out, the djangocms-versioning implementation of the plugin_renderer is not needed any more. I seem to have fixed it here: https://github.com/django-cms/django-cms/pull/7924

For testing, you can remove the plugin_renderer method from https://github.com/django-cms/djangocms-versioning/blob/master/djangocms_versioning/plugin_rendering.py .

The fix for versioning is already in its main branch and will become public with the (any day ready) release of version 2.1.

Will-Hoey commented 2 months ago

@fsbraun thanks for the update. I had just realised that it only applies to specific versions of CMS. Thanks for the speed of the fix + responses!

fsbraun commented 2 months ago

My pleasure. I want to release django CMS 4.1.2 and djangocms-versioning 2.1 soon. It would be great to get this issue fixed for both, too. Let me know your test results.

(Funny side remark: The comment in versioning's plugin_renderer refers to this issue - a typo that got me confused.)

Will-Hoey commented 2 months ago

I also noticed the typo...it had me both confused and impressed at the same time

Will-Hoey commented 2 months ago

@fsbraun patched the versioning ContentRenderer and removed the specific CMS version fixes. Works as expected so can confirm it'll be fixed next version. Will eagerly await the new releases! Thanks