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

Page_Title is "None" on root document #874

Closed
Eonasdan opened this issue Mar 25, 2016 · 15 comments
Closed

Page_Title is "None" on root document #874

Eonasdan opened this issue Mar 25, 2016 · 15 comments
Labels
Bug
Milestone

Comments

@Eonasdan
Copy link

@Eonasdan Eonasdan commented Mar 25, 2016

{{ page_title }} is displaying as "None" despite my mkdocs.yml configuration as such:

pages:
- Usage: 'index.md'
- Installing: 'Installing.md'

I just updated to version 0.15.3. This wasn't a problem with whatever version I had before. This only happens to the index page.

@waylan
Copy link
Member

@waylan waylan commented Mar 25, 2016

I'm not able to reproduce this. Which theme are you using?

@waylan
Copy link
Member

@waylan waylan commented Mar 25, 2016

Ok, I see it now. the problem does not manifest itself in the MkDocs theme without modification. Once I modified the theme (by inserting {{ page_title }} randomly in a template), the problem presented itself. I noticed that {{ page_title }} is called at mkdocs/themes/mkdocs/base.html#L13 but is wrapped in an if statement. Sure enough, on the "home" page the title is not the title of the page but the site_name. Although, in that case, I think the site_name is probably the correct thing to display -- which explains why this problem was not caught before now.

@waylan
Copy link
Member

@waylan waylan commented Mar 25, 2016

This change happened quite a while ago in 61173d1. I understand why that change was made, but I don't agree that page_title should be None for the "homepage". It should still be the "title" of the page if it has one. The template should be using a different mechanism to identify the "homepage" IMO. For example, only the "homepage" gets a page_description. That is None for all other pages. Although, page_description hardly seems like an intuitive way to identify the "homepage". Why not just include is_homepage in the context?

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Apr 24, 2016

I think you can already tell if the page is the homepage with {{ current_page.is_homepage }}. We should use that instead. Also, rather than using {{ page_title }} we should probably use {{ current_page.title }}.

@d0ugal d0ugal added Bug and removed Needs design decision labels Apr 24, 2016
@d0ugal
Copy link
Member

@d0ugal d0ugal commented Apr 24, 2016

Reclassifying as a bug, as I'm not sure what decision is needed but we should fix this 😄

@d0ugal d0ugal modified the milestone: 0.16 Apr 27, 2016
@waylan
Copy link
Member

@waylan waylan commented Apr 29, 2016

The Needs design decision label was added as I was asking for clear direction on how we want to name the various context items. Currently there is quite a bit of inconsistency. Some things which are page specific are named page_<item> (ex: page_title) while other things are just <item> (ex: content). Nothing uses current_page.<item> as per your suggestion (personally, I find "current" redundant as there only ever is one page per page).

As a reminder, while MkDocs creates an instance of a Page class for each page, and that class holds the data for a page, the instance is not passed to the template. Instead, the get_page_context function is passed the Page instance and returns a dict of the various items with inconsistent naming. And page.is_homepage is not passed to the context as all.

My recommendation would be to pass the Page instance directly to the template (potentially removing the need for the get_page_context function). However, this would be a backward incompatible change and require custom and third party templates to be changed. I suppose, for the next release we could pass in both and give users the opportunity to update their templates. Not sure how to issue a warning if a template accesses a deprecated context item though.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Apr 29, 2016

As a reminder, while MkDocs creates an instance of a Page class for each page, and that class holds the data for a page, the instance is not passed to the template.

Actually, it is passed. current_page is the page instance. This is why I suggested accessing the property that way in my previous comment via current_page.is_homepage which calls this property method. We actually use this already in both themes (read the docs, mkdocs).

I think we should move towards accessing properties rather than creating variables for everything. this will take a bit of a migration process - we could make both available in 0.16 and then remove the old in 1.0, if we can make that the next release. I think there is a hook in jinja2 to handle unknown variable names, we could probably come up with a warning/error with that.

I agree "current_" is strange, we don't need that.

We probably want to define and document a new context in detail and use that. I expect we will want a few global variables only, these spring to mind, but there might be more:

  • Page (what current_page is now)
  • Config (to access the mkdocs config)
  • Menu (all the pages)
  • Table of contents (for the current page)

(Note, I avoided giving them variable names, just starting to think of everything that we need to access).

@waylan
Copy link
Member

@waylan waylan commented Apr 29, 2016

You are taking about the nav stuff. Outside of nav the first part of your comment does not apply.

The second part is very helpful though.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Apr 29, 2016

You are taking about the nav stuff. Outside of nav the first part of your comment does not apply.

Sorry, then I must be totally confused because I don't know what this means either. This issue seems to be over my head.

@waylan
Copy link
Member

@waylan waylan commented Apr 29, 2016

Actually, it is passed. current_page is the page instance.

Well don't I feel dumb. Sorry, you are correct, I missed that. Then all we need to do is deprecate many of the other page-specific context variables and switch the templates to use the page instance.

The real issue then is how to deprecate the variables: page_title and page_description, which are redundant. And it would make sense if content, toc, meta, and canonical_url were all on the Page instance as well.

@waylan
Copy link
Member

@waylan waylan commented Apr 29, 2016

So I just went up and reread your earlier comment @d0ugal and it is now clear that I completely misread it the first time. Sorry about that. Lots of good stuff in there which is on the mark.

We probably want to define and document a new context in detail and use that. I expect we will want a few global variables only,

Right. As it currently stands, a number of config values are copied to context variables and then the config is made available as well. The config should be sufficient.

And then we have a bunch of function calls when building the context (both page and global) which use the config to piece together URLs, etc. There's no reason that can't be done in the template.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Apr 30, 2016

Sorry, I was probably vague before and confused things.

The real issue then is how to deprecate the variables: page_title and page_description, which are redundant. And it would make sense if content, toc, meta, and canonical_url were all on the Page instance as well.

I had a play with the Jinja2 API, if you put this code in the same directory as a test.html file it will print out the names of all the top level variables. I think this is all we need? It is a bit ugly, but as something we want to use only for an upgrade path and plan to remove later I think that is okay.

from jinja2 import Environment, FileSystemLoader, DebugUndefined, meta

loader = FileSystemLoader('.')
env = Environment(loader=loader, undefined=DebugUndefined)

template_source = env.loader.get_source(env,   'test.html')[0]
parsed_content = env.parse(template_source)
print meta.find_undeclared_variables(parsed_content)
@waylan
Copy link
Member

@waylan waylan commented Apr 30, 2016

Unless I'm misunderstanding something (completely possible) Jinja's undefined only works for variables which are not defined. What about the interim version were they are still defined but we want to raise a warning telling people to stop using them before the next release? I have a few untested ideas. Not sure which will work yet.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Apr 30, 2016

I think the code I have will find all variables in our case. I think the function I called is named find_undeclared_variables because variables can be defined in the template. I didn't pass any context, so there is no way it would know what we have.

So, with that code, we would look at the variables the template uses. If they match a deprecated names list we store, we then display a warning.

@waylan
Copy link
Member

@waylan waylan commented May 1, 2016

Turns out its even easier than that. Jinja has a undefined logger which issues warnings for any undefined variables used in the template. Just need to restrict that to only warn on the list we give it.

waylan added a commit to waylan/mkdocs that referenced this issue May 1, 2016
A deprecation warning is issued for all old variables and all new
page specific variables are attributes of the 'page' object.

Global variables are uneffected, except page_description.

See the changes described in the release notes for details.

Fixes mkdocs#874.
waylan added a commit to waylan/mkdocs that referenced this issue May 1, 2016
A deprecation warning is issued for all old variables and all new
page specific variables are attributes of the 'page' object.

Global variables are uneffected, except page_description.

See the changes described in the release notes for details.

Fixes mkdocs#874.
waylan added a commit to waylan/mkdocs that referenced this issue May 1, 2016
A deprecation warning is issued for all old variables and all new
page specific variables are attributes of the 'page' object.

Global variables are uneffected, except page_description.

See the changes described in the release notes for details.

Fixes mkdocs#874.
@waylan waylan closed this in #921 May 2, 2016
waylan added a commit to waylan/mkdocs that referenced this issue Aug 16, 2016
Any template variables which mirrored a config setting have been deprecated
and should be accessed via the coresponding config template variable.

This resolves the rest of mkdocs#874.
waylan added a commit to waylan/mkdocs that referenced this issue Aug 16, 2016
Any template variables which mirrored a config setting have been deprecated
and should be accessed via the coresponding config template variable.

This resolves the rest of mkdocs#874.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.