Allow plugins to provide their own Page

[squidfunk]

Closes #3366

[oprypin]

Thanks for the change.

file.page can be:

1. None (unset)
2. Something that's not a subclass of Page
3. Subclass of Page

I pushed my commit and changed the approach a bit because I think it makes more sense to distinguish "1" as the special case - "don't overwrite any object if it's there", rather than distinguishing "3" - "it's fine to overwrite something if it forgot to subclass Page".

Although we could maybe keep thinking and make "2" disallowed altogether? Not sure. Or a warning? Actually let me do that

[oprypin]

Let me know if the current shape of the PR works well for you. I'll actually release it soon, then :)

[squidfunk]

Thanks for your input!

Although we could maybe keep thinking and make "2" disallowed altogether? Not sure. Or a warning? Actually let me do that

I think we shouldn't allow that just now. It's a door to sloppyness, and is something we could add in the future without any compatibility issues if and only if it appears to be necessary for whatever reason – I currently can't think of one. Or am I missing something?

[oprypin]

I didn't understand your comment, sorry 😅

You say we shouldn't allow "that". What is meant here?

[squidfunk]

Oops, sorry for not being clear 😅 I meant we shouldn't allow file.page to be anything else than a subclass of page or None. For instance, some plugins (including ours) use checks like isinstance(file.page, Page), so if another plugin starts adding non-page objects, we can crash early. Additionally, if new methods are added to Page, subclassing makes sure that - as a plugin author – you now you might need to adapt your code.

[oprypin]

we shouldn't allow file.page to be anything else than a subclass of page or None.

Sure, but the thing is that MkDocs currently "allows" it to happen but then just overwrites it. Then the same still happens with your code. That's why I thought it's better to eventually "not allow" it - first show a warning, later break entirely.

[squidfunk]

Sure, I think a deprecation path might be a good idea. If such plugins exist, this would be a breaking change for those, but I'd say it's okay, as it's undefined behavior anyways. Adding this change, we also would uncover if there are such plugins and whether there are actually valid use cases for this edge case we currently consider unsound.

---

As an addendum : I've just started using the subclassing technique to add "special" page-like objects to MkDocs, allowing me for much better integration with MkDocs, not having to reimplement parts of it myself. Before the last rewrite of Material for MkDocs' blog plugin, the code was very imperative. Moving specific behavior into subclasses of Page turned out to be a much better design, as it also makes state observable to other plugins. You can still check for Page, or, if you have a specific plugin or hook that integrates with our plugins, you can explicitly check for those subclasses further down the chain:

from material.plugins.blog.structure import Archive

def on_page_markdown(markdown, page, **kwargs):
    if isinstance(page, Archive):
        # Page is an archive page generated by the blog plugin
        pass

[squidfunk]

So, to sum up: LGTM!

[ultrabug]

I agree with the course of the discussion and the proposed solution, thanks a lot!

[oprypin]

OK sadly this commit actually causes strange changes in behavior!
https://github.com/mkdocs/regressions/actions/runs/6114383497/job/16595834049

[oprypin]

Somehow MkDocs first populates that file with Page(title=[blank], url='user-guide/')
but then in a next iteration has to replace it with Page(title=[blank], url='/user-guide/')
-very strange

[oprypin]

The problem affects sites that use https://github.com/oprypin/mkdocs-literate-nav

First mkdocs_literate_nav creates a bogus file

  File "mkdocs/commands/build.py", line 304, in build
    files = config.plugins.on_files(files, config=config)
  File "mkdocs/plugins.py", line 533, in on_files
    return self.run_event('files', files, config=config)
  File "mkdocs/plugins.py", line 507, in run_event
    result = method(item, **kwargs)
  File "mkdocs_literate_nav/plugin.py", line 45, in on_files
    config["nav"] = resolve_directories_in_nav(
  File "mkdocs_literate_nav/plugin.py", line 108, in resolve_directories_in_nav
    result = nav_parser.resolve_yaml_nav(nav_data)
  File "mkdocs_literate_nav/parser.py", line 198, in resolve_yaml_nav
    return self._resolve_wildcards([self._resolve_yaml_nav(x) for x in nav])
  File "mkdocs_literate_nav/parser.py", line 161, in _resolve_wildcards
    self.markdown_to_nav((val.value,) + roots)
  File "mkdocs_literate_nav/parser.py", line 53, in markdown_to_nav
    dir_nav = self.get_nav_for_dir(root)
  File "mkdocs_literate_nav/plugin.py", line 93, in get_nav_for_dir
    mkdocs.structure.pages.Page(None, file, {})

Then MkDocs used to overwrite that file but now it can't

  File "mkdocs/commands/build.py", line 308, in build
    nav = get_navigation(files, config)
  File "mkdocs/structure/nav.py", line 133, in get_navigation
    items = _data_to_navigation(nav_config, files, config)
  File "mkdocs/structure/nav.py", line 189, in _data_to_navigation
    return [
  File "mkdocs/structure/nav.py", line 190, in <listcomp>
    _data_to_navigation(item, files, config)[0]
  File "mkdocs/structure/nav.py", line 182, in _data_to_navigation
    return [
  File "mkdocs/structure/nav.py", line 185, in <listcomp>
    else Section(title=key, children=_data_to_navigation(value, files, config))
  File "mkdocs/structure/nav.py", line 189, in _data_to_navigation
    return [
  File "mkdocs/structure/nav.py", line 190, in <listcomp>
    _data_to_navigation(item, files, config)[0]
  File "mkdocs/structure/nav.py", line 182, in _data_to_navigation
    return [
  File "mkdocs/structure/nav.py", line 183, in <listcomp>
    _data_to_navigation((key, value), files, config)
  File "mkdocs/structure/nav.py", line 204, in _data_to_navigation
    return Page(title, file, config)

[oprypin]

So yes this is broken: https://github.com/oprypin/mkdocs-literate-nav/blob/9904e17f3bddfa66644f96a978de523fbd9030dc/mkdocs_literate_nav/plugin.py#L93
and this looks suspicious for breakage: https://github.com/DariuszPorowski/mkdocs-file-filter-plugin/blob/b69a598e9ba75c09b63ecaa58e6c3db4eba9294c/mkdocs_file_filter_plugin/judger.py#L107

[oprypin]

So we likely need to revert this and keep brainstorming 😅
I couldn't think of any way to satisfy this use case and not break these other use cases. Well other than checking for an attribute like Page.please_actually_keep_this_page

[squidfunk]

First of all things for merging and investigating.

So the problem in the mkdocs-nav-literate plugin comes from creating pages to work around the warning that MkDocs emits when pages are not included – this could now be solved with the new file inclusion feature, right? And mkdocs-file-filter uses the page to read metadata early, which is super inefficient as files are always read twice as I can judge from the code.

Okay, so not that easy then, and it looks like users had to work around some design limitations, e.g., metadata is only made available after navigation construction, which we solve in the blog plugin by overriding the page class and moving it to the __init__ function. IMHO, this is a better approach, as we're setting read_source to a no-op, omitting the double read. Thus, I feel that subclassing is something that might be advertised on the docs for plugin authors to change behavior.

Another idea: we could only imply this logic once file.page is set to a subclass of Page. This would clearly indicate that additional logic is added (yes classes can be empty, but you get the idea), so plugin authors would explicitly opt into this behavior. I'm not sure how to check if a class is a subclass in Python, maybe something like:

if isinstance(file.page, Page) and file.page.__class__.__name__ != Page.__name__

There's probably a better approach to do that. In general, I think we need to find a better way for plugins to work together, as currently, many plugins need to work around one or the other ordering issue. If we manage to find a canonical way that we can recommend, this should go into the developer guide in the docs.

[oprypin]

Thanks. Yes there is a good way to check for the exact type. For this release, let's limit this ability only to strict subclasses. That's not a nice solution but it's the only thing we can reasonably do on short notice. I'll send such a pull request.