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

Add failsafe for permalinksToUrls method if uuid does not exist #6165

Closed
SebastianEberlein-JUNO opened this issue Jan 17, 2024 · 4 comments · Fixed by #6313
Closed

Add failsafe for permalinksToUrls method if uuid does not exist #6165

SebastianEberlein-JUNO opened this issue Jan 17, 2024 · 4 comments · Fixed by #6313
Labels
type: bug 🐛 Is a bug; fixes a bug
Milestone

Comments

@SebastianEberlein-JUNO
Copy link

SebastianEberlein-JUNO commented Jan 17, 2024

Description

Example: A text block has an inline link to a page. If the linked page is deleted and the uuid does not exist anymore, the following error is displayed:

Block error: "DOMElement::setAttribute(): Passing null to parameter #2 ($value) of type string is deprecated" in block type: "text"

Because in the permalinksToUrls method, the $url in this line is null:
https://github.com/getkirby/kirby/blob/main/config/methods.php#L488

Your setup

Kirby Version
4.0.3

@SebastianEberlein-JUNO SebastianEberlein-JUNO changed the title Add failsafe for permalinksToUrls method Add failsafe for permalinksToUrls method if uuid does not exist Jan 17, 2024
@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Jan 22, 2024
@distantnative distantnative added this to the 4.2.0 milestone Jan 22, 2024
@medienbaecker
Copy link
Contributor

medienbaecker commented Jan 23, 2024

I just noticed the same issue outside of blocks. In this case the whole website went down because a page was deleted. I'd personally classify this as a bug instead of an enhancement.

To reproduce

  1. Add a writer field to your blueprint and use permalinksToUrls() in the frontend
  2. Link a page
  3. Delete the page

Edit:
I just saw the "This method is still experimental" disclaimer in the code, so I can understand where the "enhancement" is coming from. It would be great if this disclaimer could be added to the method docs.

@medienbaecker
Copy link
Contributor

I recently discussed this issue with @mrflix and he rightfully suggested it might help to talk about the expected behaviour:

The (link: ) tag deals with this in a very elegant way in my opinion. When debug mode is on, an exception is thrown, otherwise the error page's URL is used. I'd expect the permalinksToUrls() method to work in a similar way.

In the meantime, as mentioned in my previous comment, I think the Kirby community would really appreciate a disclaimer in the docs. Not everyone looks at the source code and the method is there for a reason.

@bvdputte
Copy link

I'ld categorize this as a bug as well as it crashes page rendering in unexpected situations.

Is there any update on this?

@distantnative distantnative added type: bug 🐛 Is a bug; fixes a bug and removed type: enhancement ✨ Suggests an enhancement; improves Kirby labels Feb 27, 2024
@bastianallgeier
Copy link
Member

Sorry that it took so long. This is definitely a bug. I just submitted a PR for it #6313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants