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

Loading hyperref with \AddToHook{begindocument} #242

Closed
DrDaum opened this issue Jun 1, 2022 · 7 comments
Closed

Loading hyperref with \AddToHook{begindocument} #242

DrDaum opened this issue Jun 1, 2022 · 7 comments

Comments

@DrDaum
Copy link

DrDaum commented Jun 1, 2022

When loading the latest version of hyperref with \AddToHook{begindocument} "Missing \endcsname inserted" comes up when using \ref twice:

\documentclass{article}

\AddToHook{begindocument}{ 
	\RequirePackage{hyperref}
}

\begin{document}
Table~\ref{tab1}\\
Table~\ref{tab2}


Caption\label{tab1}\\
Caption\label{tab2}

\end{document}

image

@muzimuzhi
Copy link
Contributor

Caused by 32a9c4d#diff-d5dc5003cc3fc59b9302ead2d25eb3fe8bc437ce5882facaffe0830cb8c5222cR674 which postpones redefinition of \ref to \AtBeginDocument in nameref.sty.

When hyperref is loaded with \AtBeginDocument/\AddToHook{begindocument}, \AtBeginDocument is the same as \@firstofone hence redefinition of \ref is executed immediately, not postponed. In short, when hyperref is loaded with \AtBeginDocument, you get an illy-defined \ref thus the error.

In general it's a bad idea, hence dangerous, to load any package with \AtBeginDocument. Whether such use case works or not totally depends on the implementation of package and I doubt if it's ever officially supported.

@scottkosty
Copy link

@muzimuzhi thank you for your explanation. I think I might have found a similar issue with EuropeCV. I reported it there.

@davidcarlisle
Copy link
Member

@scottkosty It looks like europecv could be modified to use begindocument/before hook so not quite as late as begindocument also removing the obsolete UTF-8 handling

$ diff europecv.cls~ europecv.cls
$ diff europecv.cls~ europecv.cls
421,422c421,423
< \RequirePackage{ucs}
< \RequirePackage[utf8x]{inputenc}
---
> %\RequirePackage{ucs}
> %\RequirePackage[utf8x]{inputenc}
> \renewcommand\ecv@utf[1]{{#1}}
449c450
< \AtBeginDocument{%
---
> \AddToHook{begindocument/before}{%

@scottkosty
Copy link

@davidcarlisle Thank you! I confirm that fixes things from what I can see. Before I make a pull request, I want to make sure I understand one thing and possibly put it in the release notes. If a user had the line \usepackage[utf8x]{inputenc} in their preamble, their document that used to compile will now fail. But it is in some sense a "good failure" because the user should update their code to avoid the not-recommended utf8x anyway. Is that a reasonable interpretation?

Can you copy/paste your comment above into an answer to
https://tex.stackexchange.com/questions/646747/europecv-fails-with-latex2e-pre-release-because-of-loading-hyperref-in-atbegind ?

Thanks!

@davidcarlisle
Copy link
Member

@scottkosty as I think @u-fischer commented elsewhere we may be able to arrange that utf8x plays more nicely with hyperref, but it is very rarely needed these days so a class file like europecv shouldn't load it, then if an individual document does need that they can use the patch described in latex3/latex2e#833 if hyperref has not yet incorporated a similar patch.

@scottkosty
Copy link

@davidcarlisle That all sounds good. Thanks for your patient explanations!

@u-fischer
Copy link
Member

I close this as resolved. As @muzimuzhi wrote it is a bad idea to load packages in the begindocument. Various packages initialize commands in this hook (including hyperref itself) and timing can get quite tricky if a package is loaded so late.
The better hook here is begindocument/before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants