Skip to content

Comments

Fix section index partial NPE#1890

Merged
chalin merged 1 commit intogoogle:mainfrom
tobiaskohlbau:issue/1874
Apr 25, 2024
Merged

Fix section index partial NPE#1890
chalin merged 1 commit intogoogle:mainfrom
tobiaskohlbau:issue/1874

Conversation

@tobiaskohlbau
Copy link
Contributor

@tobiaskohlbau tobiaskohlbau commented Mar 11, 2024

During rendering the section index of a docs list it could happen that a file has no parent for e.g. the homepage. If such scenario is existing the .Parent.File of a file is nil. Therefore this case should be handled within the rendering itself.

Fixes #1874

@tobiaskohlbau tobiaskohlbau mentioned this pull request Mar 11, 2024
Closed
2 tasks
deining
deining suggested changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@deining deining left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just ran into the same issue, and the fix proposed with this PR solved my problem.
I realized that this contribution proposes a slight more elegant solution to the problem .
@tobiaskohlbau: can you have a look, please? Can we take over this solution here?

@tobiaskohlbau
Copy link
Contributor Author

tobiaskohlbau commented Mar 18, 2024 •
edited
Loading

Just ran into the same issue, and the fix proposed with this PR solved my problem. I realized that this contribution proposes a slight more elegant solution to the problem . @tobiaskohlbau: can you have a look, please? Can we take over this solution here?

AFAIK the proposed solution would not work. As the and within the if only checks for the currently scoped Parent but not for every pages Parent.File.UniqueID. Therefore if pages do not have a Parent.File the access to UniqueID would throw a NPE. It's required to filter out all pages where Parent.File=nil. (IIRC I had such solution before and it still got said NPE.)

@yann-soubeyrand
Copy link
Contributor

yann-soubeyrand commented Mar 24, 2024

Hello,

I confirm what @tobiaskohlbau said.

Using Hugo 0.124.1, which contains the fix referred to above, on a docs only site, that is with the following front matter in content/_index.adoc

title: 'Documentation'
layout: 'section'

cascade:
 - type: 'docs'

I still get a warning:

WARN  .File.UniqueID on zero object. Wrap it in if or with: {{ with .File }}{{ .UniqueID }}{{ end }}

But the warning is solved when adding the modified partial from this PR.

@yann-soubeyrand
Copy link
Contributor

yann-soubeyrand commented Mar 25, 2024

I re-tested today and, in fact, this is not a warning, but an error:

ERROR render of "taxonomy" failed: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/[email protected]/layouts/docs/list.html:12:5": execute of template failed: template: docs/list.html:12:5: executing "main" at <partial "section-index.html" .>: error calling partial: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/[email protected]/layouts/partials/section-index.html:8:69": execute of template failed: template: partials/section-index.html:8:69: executing "partials/section-index.html" at <$parent.File.UniqueID>: error calling UniqueID: runtime error: invalid memory address or nil pointer dereference
Built in 3662 ms
Error: error building site: render: failed to render pages: render of "taxonomy" failed: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/[email protected]/layouts/docs/list.html:12:5": execute of template failed: template: docs/list.html:12:5: executing "main" – File is nil; wrap it in if or with: {{ with partial "section-index.html" .>: error calling partial: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/[email protected]/layouts/partials/section-index.html:8:69": execute of template failed: template: partials/section-index.html:8:69: executing "partials/section-index.html" at <.File }}{{ .UniqueID }}{{ end }}

I still confirm this PR fixes the issue.

@deining
Copy link
Contributor

deining commented Apr 7, 2024

AFAIK the proposed solution would not work. As the and within the if only checks for the currently scoped Parent but not for every pages Parent.File.UniqueID. Therefore if pages do not have a Parent.File the access to UniqueID would throw a NPE. It's required to filter out all pages where Parent.File=nil. (IIRC I had such solution before and it still got said NPE.)

You are right. Your solution is more general and should be merged unchanged.

@deining deining added this to the 24Q2 milestone Apr 7, 2024
@deining deining mentioned this pull request Apr 7, 2024
Closed
21 tasks
chalin
chalin reviewed Apr 7, 2024
View reviewed changes
layouts/partials/section-index.html
{{ $pages = (where $pages ".Parent" "!=" nil) -}}
{{ if and .Parent .Parent.File -}}
{{ $pages = (where $pages ".Parent.File" "!=" nil) -}}
{{ if and $parent $parent.File -}}
Copy link
Collaborator

@chalin chalin Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at this more closely, it seems to make sense indeed.

Btw, the $parent variable is misnamed, because$parent is set to .Page.

Can we inline the variable instead?

Btw, IMHO .Page cannot be falsy, but .Page.File could be falsy. So the if condition can be simplified.

Copy link
Collaborator

@chalin chalin Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will track separately:

@tobiaskohlbau @chalin
fix issue with section index partial
3933271 
During rendering the section index of a docs list it could happen
that a file has no parent for e.g. the homepage. If such scenario is
existing the .Parent.File of a file is nil. Therefore this case should
be handled within the rendering itself.

Fixes google#1874

Signed-off-by: Tobias Kohlbau <[email protected]>
chalin
chalin approved these changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm willing to give this a try.
RSLGTM based on appraisals by @deining and @yann-soubeyrand.
Thanks all! ✨

@chalin chalin mentioned this pull request Apr 25, 2024
Closed
@chalin chalin changed the title fix issue with section index partial Fix section index partial NPE Apr 25, 2024
@chalin chalin merged commit 3be4075 into google:main Apr 25, 2024
@chalin chalin mentioned this pull request Apr 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chalin chalin chalin approved these changes

+1 more reviewer

@deining deining deining approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

24Q2

Development

Successfully merging this pull request may close these issues.

NPE for $parent.File.UniqueID using Hugo 0.123.1.

4 participants

@tobiaskohlbau @yann-soubeyrand @deining @chalin