Allow dirhtml builder without `ogp_site_url`

rkdarst commented 1 year ago

Yes, the readme says that ogp_site_url is very important. And indeed, reports online say that OGP doesn't work with relative URLs. Although at least one place says it does...
Yet, I got confused when I could build it with html (it left everything relative), but not with dirhtml (obscure extension error it took me a while to figure out). (And it only fails on the page named /index.
I tried several different things, like first adding an exception when ogp_site_url is unset. But then I realized what the root issue was, and thought that something less invasive, keeping existing behavior, even if it wasn't perfect. Then we can see where to go from there.
Even if this isn't perfect, it helps a bit with "make this extension do something useful, even if not perfect, if it's installed but not configured".
This change makes /index work with dirhtml without any configuration, since urljoin works with None as a first argument to leave the URLs as relative. This seemed like the the most consistent and least invasive change.
So overall, even though this isn't a perfect change, and could be considered technically incorrect, I think it's less confusing and makes things more consistent for a possibly better solution later.

hugovk commented 1 year ago

Please can you give an example of how exactly it fails with dirhtml?

rkdarst commented 1 year ago

I saw those, looking again it seems the problem was introduced in #78.

It errors with this:

Extension error (sphinxext.opengraph):
Handler <function html_page_context at 0x7f98118190d0> for event 'html-page-context' threw an exception (exception: 'NoneType' object has no attribute 'replace')

Or a full traceback:

Traceback (most recent call last):
  File "/home/xxx/venv/lib/python3.9/site-packages/sphinx/events.py", line 94, in emit
    results.append(listener.handler(self.app, *args))
  File "/home/yyy/sphinxext-opengraph/sphinxext/opengraph/__init__.py", line 192, in html_page_context
    context["metatags"] += get_tags(app, context, doctree, app.config)
  File "/home/yyy/sphinxext-opengraph/sphinxext/opengraph/__init__.py", line 176, in get_tags
    [make_tag(p, c) for p, c in tags.items()]
  File "/home/yyy/sphinxext-opengraph/sphinxext/opengraph/__init__.py", line 176, in <listcomp>
    [make_tag(p, c) for p, c in tags.items()]
  File "/home/yyy/sphinxext-opengraph/sphinxext/opengraph/__init__.py", line 34, in make_tag
    content = content.replace('"', "&quot;")
AttributeError: 'NoneType' object has no attribute 'replace'

The root problem was a call make_tag(property='og:url', content=None) and then content is not a string so you can't content.replace(...). That None comes from the line I changed - before #78, it would always be a string for dirhtml. And due to a quirk in how it works now when it's not configured with an absolute url, if ogp_site_url was None, the function gets passed None instead of '' which would make a (little bit) more sense as a default value.

Other possible solutions:

Add error if ogp_site_url is not set (I think minimally working with no configuration is still useful, it does provide some useful description and titles)
modify make_tag to support content=None
set the default value of ogp_site_url to ''.

TheTripleV commented 1 year ago

I think

set the default value of ogp_site_url to ''

is fine to restore previous behavior.

rkdarst commented 1 year ago

Updated and tested on my own site. Found and fixed one unintended consequence, but not sure if there could be others - doesn't seem so.

hugovk commented 1 year ago

The root problem was a call make_tag(property='og:url', content=None) and then content is not a string so you can't content.replace(...). That None comes from the line I changed - before #78, it would always be a string for dirhtml. And due to a quirk in how it works now when it's not configured with an absolute url, if ogp_site_url was None, the function gets passed None instead of '' which would make a (little bit) more sense as a default value.

Ah right, I had ogp_site_url set when testing #78. Can repro when removing it, and it works again with this PR. 👍

Maybe add some test cases to this PR to help avoid regressions in the future?

rkdarst commented 1 year ago

thanks for this and the other one!

rkdarst commented 1 year ago

So, I see that I forgot to git add the no-configuration test root. Would you like me to add this and change the test to use it? - I must have thought that was even more simple and less-configured than the readthedocs one

TheTripleV commented 1 year ago

I think it's ok. The rtd default one has no rtd or opengraph specific configuration.

wpilibsuite / sphinxext-opengraph

Allow dirhtml builder without `ogp_site_url` #84