Allow dirhtml builder without ogp_site_url

[rkdarst]

• 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]

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

(See also #77 and #78)

[rkdarst]

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]

I think

set the default value of ogp_site_url to ''

is fine to restore previous behavior.

[rkdarst]

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]

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]

thanks for this and the other one!

[rkdarst]

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]

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