Re: To keep or not to keep: logging of undefined template variables
I was going to have a go at this ticket (https://code.djangoproject.com/ticket/28526) - which links this thread. Having read the various replies, it seems like there is no shortage of differing views how missing/invalid variables should be treated: - There should be no logging - There should be logging, but full tracebacks are too noisy / not beginner friendly - Full tracebacks are useful - There should be logging but only for the first time an invalid variable is encountered within a template - Some combination of the above with a setting (or settings) to alter behaviour - There should be a setting to make invalid variables raise errors There is a danger of bloating the number of options/settings and still not catering for what everyone wants. One way to allow all of these would be to provide a hook to handle raising errors/logging in the case of invalid variables. This would also allow all kinds of custom behaviour, that might be useful for particular cases, but which wouldn't warrant being features of django itself: - log / raise only on particular templates - provide different levels of logging depending on the type of the invalid variable - etc I originally thought of replacing `string_if_invalid` with an option function `invalid_variable_handler` that points to a function, which would return a string for invalid variables, and could also handle logging/errors at the same time. Perhaps this is too many concerns for one function; although having said that, it's really just one concern - handling invalid/undefined variables. The default function could return `string_if_invalid` during the interim whilst we add a deprecation warning for `string_if_invalid`. A less destructive option would be to leave `string_if_invalid` as it is, and just move the code that logs/raises errors to a public method `handle_variable_resolve_errors` of Engine(?). This would provide a public API for people to customise logging/raising of errors if they should so wish, by sub-classing `Engine` (and then add an engine attribute to DjangoTemplates??). Alternatively, `handle_variable_resolve_errors` could just be another option which would point to the above function, rather than having to subclass Engine. The very first of these approaches would allow useful things like returning different `string_if_invalid` values for different templates, if debug is on etc. But seems a bit messier. There are a few more variations on the same theme, in terms of actual implementation, but the basic idea is the same - move logging/raising errors to an overridable function. Any thoughts on this would be much appreciated On Friday, August 25, 2017 at 11:50:27 AM UTC+1 Vlastimil Zíma wrote: > If anyone is interested, I've cleaned the errors in admin templates: > > Ticket: https://code.djangoproject.com/ticket/28529 > PR: https://github.com/django/django/pull/8973 > > The fixes are quite simple. The biggest problem is sometimes to find out, > in which template the bug actually appears. > > Vlastik > > Dne pátek 25. srpna 2017 9:28:30 UTC+2 Vlastimil Zíma napsal(a): > >> Apparently there is number of errors in admin templates. I suggest to fix >> the templates. I my experience, the most cases are missing if statements or >> missing context variables. These can be fixed very easily and produce >> cleaner templates. I consider this much better solution than just ignoring >> error messages. >> >> As Anthony suggested, the main problem is more often the fuzziness of the >> messages, which do not often properly state template, line or expression >> which is incorrect. This makes it difficult to resolve them in some cases. >> >> Vlastik >> >> Dne čtvrtek 24. srpna 2017 17:21:38 UTC+2 Tim Graham napsal(a): >>> >>> We received a report that shows the large number of undefined variable >>> warnings when rendering an admin changelist page [0]. >>> >>> I'm still not sure what the solution should be, but I created #28526 [1] >>> to track this problem: finding a remedy to the problem of verbose, often >>> unhelpful logging of undefined variables. >>> >>> I don't think "the end goal of errors raising when using undefined >>> variables" is feasible. My sense is that relying on the behavior of >>> undefined variables is too entrenched in the Django template language to >>> change it at this point. (If someone wanted to try to fix all the warnings >>> in the admin templates, that might provide a useful data point). See the >>> "Template handling of undefined variables" thread [2] for a longer >>> discussion. >>> >>> [0] https://code.djangoproject.com/ticket/28516 >>> [1] https://code.djangoproject.com/ticket/28526 >>> [2] >>> https://groups.google.com/d/topic/django-developers/LT5ESP0w0gQ/discussion >>> >>> On Tuesday, June 20, 2017 at 4:12:52 AM UTC-4, Anthony King wrote: -1 for removing logs. Like Vlastimil, it's helped me spot a couple of stray bugs. What I'd actually like
Re: Difference between AdminSite.admin_view and staff_member_required?
staff_member_required feels like a bit of a relic from the past: - It looks like before a massive refactor of the admin that happened 12 years ago ( https://github.com/django/django/commit/a19ed8aea395e8e07164ff7d85bd7dff2f24edca) staff_member_required played a similar role to what Adminsite.admin_view does now (checks permissions and redirects to login if appropriate). Before the refactor, all of the relevant views were wrapped with staff_member_required. - The above commit introduced AdminSite which has its own 'has_permissions' method. Adminsite.root (which was the old get_urls) called self.has_permissions before calling any other views, so staff_member_required was now no longer really required. - staff_member_required did however continue to be used in the admindocs app (which was now split off into its own app). - Adminsite.root was eventually replaced with get_urls and the call to has_permissions was moved into `Adminsite.admin_view` (which also does a few other things: adds csrf protection, and deals with caching) - Over time staff_member_required became simpler and simpler as things like user_passes_test were introduced, and there was no need to repeat logic. - It's now just a very specific version of `user_passes_test` and is only used internally one place - the admindocs app. Tobias - In terms of permissions and redirecting; the behaviour of admin_view and `staff_member_required` are very similar. The differences are that: i) admin_view defers to Adminsite.has_permission to do the actual checking of permissions (so if that method is overwritten the behaviour will change) ii) admin_view does a whole load of other things that all admin views are going to need (mentioned above). iii) They have slightly different arguments you can pass in to modify the behaviour If you are decorating a view of ModelAdmin, or AdminSite you should use admin_view - as described in the docs under get_urls. Since staff_member_required is more or less a very specific version of user_passes_test designed for admin views, it's use-case seems pretty niche. If you were making an app like admindocs which behaves like an extension of the admin app, without actually being part of the admin app you could use it then, but I'm struggling to think of any other time you would want to use it though. I think there's an argument to be made for deprecating it completely: - It's only used in one place internally, and the wider use case seems pretty niche. - user_passes_test can do the same thing and seems to be the standard way of doing this kind of thing anyway. - user_passes_test with a suitably named test argument would also be more explicit, since the login_url and redirect_field_name would need to be explicitly stated. Definitely the documentation should be updated though. Thoughts on deprecating it? Tim On Tue, Mar 9, 2021 at 7:08 PM Tobias Bengfort wrote: > On 09/03/2021 18.03, Carlton Gibson wrote: > > Do you want to open a PR with suggested changes as a focus for > discussion? > > I would open a PR that improves the docs, but I still don't understand > the difference, so I wouldn't know what to write. > > The other alternative would be remove one of them, but at this point > that is probably too drastic. > > tobias > > -- > You received this message because you are subscribed to the Google Groups > "Django developers (Contributions to Django itself)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to django-developers+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/d10799d4-8a42-6caa-8e13-c2ce93256521%40posteo.de > . > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFBPPP0bM2cr9etcMQwmprK1zX8gfKZ2wmEa7BW2CyiCrxTUog%40mail.gmail.com.
Re: Difference between AdminSite.admin_view and staff_member_required?
> It seems like a good idea. However, it leads us to another question. Do we really need the is_staff field on the User Model if it is deprecated? User.is_staff is still used, just from Adminsite.has_permission instead: https://github.com/django/django/blob/main/django/contrib/admin/sites.py#L196 On Tue, Mar 9, 2021 at 9:45 PM Matthew Pava wrote: > > Thoughts on deprecating it? > > It seems like a good idea. However, it leads us to another question. Do we > really need the is_staff field on the User Model if it is deprecated? > > > > *From:* django-developers@googlegroups.com < > django-developers@googlegroups.com> *On Behalf Of *Timothy McCurrach > *Sent:* Tuesday, March 9, 2021 3:22 PM > *To:* django-developers@googlegroups.com > *Subject:* Re: Difference between AdminSite.admin_view and > staff_member_required? > > > > staff_member_required feels like a bit of a relic from the past: > > - It looks like before a massive refactor of the admin that happened 12 > years ago ( > https://github.com/django/django/commit/a19ed8aea395e8e07164ff7d85bd7dff2f24edca > <https://us-east-2.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL2RqYW5nby9kamFuZ28vY29tbWl0L2ExOWVkOGFlYTM5NWU4ZTA3MTY0ZmY3ZDg1YmQ3ZGZmMmYyNGVkY2E=&i=NWVjN2YxNzUxNGEyNzMxNmMyMGRkZGU1&t=YkNVTDFoOEQ4dEdQdU1PYW9WWlJ0R0R0Y2ZGN0hRYkRiT0FkNTRkaU1Ocz0=&h=64eb5f1d05c6477aa783f14819c9eebe>) > staff_member_required played a similar role to what Adminsite.admin_view > does now (checks permissions and redirects to login if appropriate). Before > the refactor, all of the relevant views were wrapped with > staff_member_required. > - The above commit introduced AdminSite which has its own > 'has_permissions' method. Adminsite.root (which was the old get_urls) > called self.has_permissions before calling any other views, so > staff_member_required was now no longer really required. > - staff_member_required did however continue to be used in the admindocs > app (which was now split off into its own app). > - Adminsite.root was eventually replaced with get_urls and the call to > has_permissions was moved into `Adminsite.admin_view` (which also does a > few other things: adds csrf protection, and deals with caching) > - Over time staff_member_required became simpler and simpler as things > like user_passes_test were introduced, and there was no need to repeat > logic. > - It's now just a very specific version of `user_passes_test` and is only > used internally one place - the admindocs app. > > > Tobias - In terms of permissions and redirecting; the behaviour of > admin_view and `staff_member_required` are very similar. The differences > are that: > i) admin_view defers to Adminsite.has_permission to do the actual > checking of permissions (so if that method is overwritten the behaviour > will change) > ii) admin_view does a whole load of other things that all admin views are > going to need (mentioned above). > iii) They have slightly different arguments you can pass in to modify the > behaviour > > If you are decorating a view of ModelAdmin, or AdminSite you should use > admin_view - as described in the docs under get_urls. > > Since staff_member_required is more or less a very specific version of > user_passes_test designed for admin views, it's use-case seems pretty > niche. If you were making an app like admindocs which behaves like an > extension of the admin app, without actually being part of the admin app > you could use it then, but I'm struggling to think of any other time you > would want to use it though. > > I think there's an argument to be made for deprecating it completely: > - It's only used in one place internally, and the wider use case seems > pretty niche. > - user_passes_test can do the same thing and seems to be the standard way > of doing this kind of thing anyway. > - user_passes_test with a suitably named test argument would also be more > explicit, since the login_url and redirect_field_name would need to be > explicitly stated. > > Definitely the documentation should be updated though. Thoughts on > deprecating it? > > Tim > > > > On Tue, Mar 9, 2021 at 7:08 PM Tobias Bengfort > wrote: > > On 09/03/2021 18.03, Carlton Gibson wrote: > > Do you want to open a PR with suggested changes as a focus for > discussion? > > I would open a PR that improves the docs, but I still don't understand > the difference, so I wouldn't know what to write. > > The other alternative would be remove one of them, but at this point > that is probably too drastic. > > tobias > > -- > You received this message because you are subscribed to the Google Groups > "Django
