Repository: mkdocs/mkdocs

Issue:
Allow plugins to override `Page` when populating navigation
Following up on https://github.com/squidfunk/mkdocs-material/issues/5909#issuecomment-1696958973, I'm suggesting to adjust the logic that MkDocs implies to create `Page` objects from/in files, i.e., `File` objects. Currently, [`_data_to_navigation`](https://github.com/mkdocs/mkdocs/blob/master/mkdocs/structure/nav.py#L179) will always create (and thus overwrite) `Page` on everything that doesn't look like a `Link` or `Section`. Excerpt of the comment linked:

> MkDocs will always generate either a [`Link` or a `Page` for a file that is not excluded](https://github.com/mkdocs/mkdocs/blob/94e9f17cd7a69e70c18ae282d55ba2f34f93c542/mkdocs/structure/nav.py#L203-L204). As you've probably noticed, the blog plugin makes use of subclasses for pages of specific types, e.g. [`Post`](https://github.com/squidfunk/mkdocs-material/blob/de0c5b7719deff6d092dc673e82af701c6f36001/src/plugins/blog/structure/__init__.py#L48), [`View`](https://github.com/squidfunk/mkdocs-material/blob/de0c5b7719deff6d092dc673e82af701c6f36001/src/plugins/blog/structure/__init__.py#L204), etc., and relations among different concepts in the plugin are managed inside the instances of those subclasses. The problem is, that __MkDocs will always generate a plain old `Page` instance, overwriting the more specific instance that has been generated by the plugin before__ [after `on_files` and before `on_nav`](https://github.com/mkdocs/mkdocs/blob/94e9f17cd7a69e70c18ae282d55ba2f34f93c542/mkdocs/commands/build.py#L303-L311). We could still solve it by splitting file generation and page instantiation, but this would make things much more complicated and non-linear, because during generation, [we already assign categories to pages and vice versa](https://github.com/squidfunk/mkdocs-material/blob/de0c5b7719deff6d092dc673e82af701c6f36001/src/plugins/blog/plugin.py#L598-L599), etc. We would need to split this into two stages, blowing up the code base of the plugin and also the runtime, as we're doing two passes. This is clearly not favorable from our perspective.
>
> __Moving forward__: I believe that this problem can be mitigated with a very small but important change in MkDocs: if a `File` already defines a dedicated `Page` in `file.page`, MkDocs should just use that instead of creating a new one:
> 
> ``` py
> return file.page if isinstance(file.page, Page) else Page(title, file, config)
> ```
>
> It's essentially a single line change in MkDocs that would make things much, much simpler for plugin developers. I've written several plugins now, and I repeatedly had to work around the oddities of this behavior.

Thus, I'm proposing to change [the following line](https://github.com/mkdocs/mkdocs/blob/94e9f17cd7a69e70c18ae282d55ba2f34f93c542/mkdocs/structure/nav.py#L203):

``` py
# Now: always create a page
return Page(title, file, config)

# Better: only create a page when one was not created before
return file.page if isinstance(file.page, Page) else Page(title, file, config)
```

Currently, the only way to work around this limitation is to overwrite `Page.__class__` on the resulting instance in `on_nav`, e.g., like done in [section-index](https://github.com/oprypin/mkdocs-section-index/blob/f5414c0775848cec96f1181f74c622fe3e14985e/mkdocs_section_index/plugin.py#L33-L34), which, AFAIK, does not call `__init__` or initializes instance variables:

``` py
page.__class__ = CustomPage
assert isinstance(page, CustomPage)
```

Discussion:
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:

``` python
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!

Patch:
diff --git a/mkdocs/structure/nav.py b/mkdocs/structure/nav.py
index e6744190..f01d9176 100644
--- a/mkdocs/structure/nav.py
+++ b/mkdocs/structure/nav.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import logging
+import warnings
 from typing import TYPE_CHECKING, Iterator, TypeVar
 from urllib.parse import urlsplit
 
@@ -200,6 +201,12 @@ def _data_to_navigation(data, files: Files, config: MkDocsConfig):
                 f"A reference to '{file.src_path}' is included in the 'nav' "
                 "configuration, but this file is excluded from the built site.",
             )
+        if file.page is not None:
+            if not isinstance(file.page, Page):
+                warnings.warn(  # type: ignore[unreachable]
+                    "File.page should not be set to any type other than Page", DeprecationWarning
+                )
+            return file.page
         return Page(title, file, config)
     return Link(title, path)
 


Test patch:
diff --git a/mkdocs/tests/structure/nav_tests.py b/mkdocs/tests/structure/nav_tests.py
index 3e4aba5f..44ae2adc 100644
--- a/mkdocs/tests/structure/nav_tests.py
+++ b/mkdocs/tests/structure/nav_tests.py
@@ -378,6 +378,34 @@ class SiteNavigationTests(unittest.TestCase):
         self.assertEqual(len(site_navigation.pages), 7)
         self.assertEqual(repr(site_navigation.homepage), "Page(title=[blank], url='/')")
 
+    def test_nav_page_subclass(self):
+        class PageSubclass(Page):
+            pass
+
+        nav_cfg = [
+            {'Home': 'index.md'},
+            {'About': 'about.md'},
+        ]
+        expected = dedent(
+            """
+            PageSubclass(title=[blank], url='/')
+            PageSubclass(title=[blank], url='/about/')
+            """
+        )
+        cfg = load_config(nav=nav_cfg, site_url='http://example.com/')
+        fs = [
+            File(list(item.values())[0], cfg.docs_dir, cfg.site_dir, cfg.use_directory_urls)
+            for item in nav_cfg
+        ]
+        files = Files(fs)
+        for file in files:
+            PageSubclass(None, file, cfg)
+        site_navigation = get_navigation(files, cfg)
+        self.assertEqual(str(site_navigation).strip(), expected)
+        self.assertEqual(len(site_navigation.items), 2)
+        self.assertEqual(len(site_navigation.pages), 2)
+        self.assertEqual(repr(site_navigation.homepage), "PageSubclass(title=[blank], url='/')")
+
     def test_active(self):
         nav_cfg = [
             {'Home': 'index.md'},