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

fix Exception while resolving variable 'toolbar' in template #1906

Closed
wants to merge 1 commit into from

Conversation

foundkey
Copy link

Description

When sending a request using ajax, toolbar.js will send a request at the same time:
GET /__debug__/history_sidebar/?store_id=<id> HTTP/1.1

This request will throw the following exception when rendering the template.
It seems because toolbar is not passed in panel_context.

Fixes # (#1905 )

  1. add toolbar into panel_context.

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@matthiask
Copy link
Member

I think this makes sense, but I'm not 100% sure if you're just fixing an issue with a missing variable or if the functionality of loading panel contents with RENDER_PANELS = True and older requests was broken previously.

If it's the former I'm good with the PR, but if it's the later we'd really want a test to make sure we're not regressing here.

@foundkey
Copy link
Author

I think this makes sense, but I'm not 100% sure if you're just fixing an issue with a missing variable or if the functionality of loading panel contents with RENDER_PANELS = True and older requests was broken previously.

If it's the former I'm good with the PR, but if it's the later we'd really want a test to make sure we're not regressing here.

My local test was fine. After modification, no exception logs can be seen in the same scenario.
However, I only tested the scenarios I encountered, and I did not conduct a comprehensive test.

Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying a solution! Would you be able to include a test that confirms that this is fixed? It's hard to accept a change without being able to see what it's fixing.

@foundkey
Copy link
Author

Thanks for identifying a solution! Would you be able to include a test that confirms that this is fixed? It's hard to accept a change without being able to see what it's fixing.

Sorry, this test doesn't look easy to add.

According to the Django Template rendering process, the exception django.template.base.VariableDoesNotExist is handled gracefully. This exception is not thrown and is replaced with a default value, which is an empty string if not configured.
template_resolve

This results in an empty string being obtained at the exception and going to the false branch, which is in line with the expected logic.
debug-toolbar-exception

According to debug-toolbar's code:
if panel_content.html is rendered under a non-ajax request, there will be a toolbar variable in the context, which is normal.
If panel_content.html is rendered under an ajax request, the toolbar variable is missing from the context, eventually leading to the false branch. In ajax request, toolbar.should_render_panels value is always false.

It seems that this exception does not trigger any problems. Only in debug level logs, you will see relevant error logs.
To sum up, it won't hurt if this anomaly is not fixed.

@foundkey
Copy link
Author

After turning on Debug level logging, VariableDoesNotExist errors appear frequently during use. But it does not affect the use. I think my PR is redundant. It can be closed.

@foundkey foundkey closed this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants