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

Duplicate content when appending .html to page URLs #6509

Open
lukasbestle opened this issue Jun 26, 2024 · 7 comments
Open

Duplicate content when appending .html to page URLs #6509

lukasbestle opened this issue Jun 26, 2024 · 7 comments
Labels
type: bug 🐛 Is a bug; fixes a bug
Milestone

Comments

@lukasbestle
Copy link
Member

lukasbestle commented Jun 26, 2024

Description

Kirby renders the same page if .html is appended to the page URL as if no content representation is appended at all.

Expected behavior

.html should behave like .somethinginvalid and render a 404 unless a template .html.php actually exists.

To reproduce

Visit https://getkirby.com/docs/guide/quickstart and https://getkirby.com/docs/guide/quickstart.html

Your setup

Kirby Version

4.3.0

Additional context

I believe this is caused by $page->render(array $data = [], $contentType = 'html'). Because $contentType defaults to html if no content representation was called, it cannot differentiate between page.html and page. In both cases it gets $contentType = 'html'.

So to fix this, we would need to change the default to string|null $contentType = null. Only if empty($contentType) === true, the main template would be loaded. html would then load a .html.php template if it exists.

This would be a breaking change, so v5 would be a good candidate.

@lukasbestle lukasbestle added the type: bug 🐛 Is a bug; fixes a bug label Jun 26, 2024
@lukasbestle lukasbestle added this to the 5.0.0 milestone Jun 26, 2024
@afbora
Copy link
Member

afbora commented Jul 3, 2024

I think this is more complex than you think. I think this is also and more related with Template class. Template::$type and Template::$defaultType accept html as default. So Template::hasDefaultType() always returns true and load default template.

https://github.com/getkirby/kirby/blob/4.3.0/src/Template/Template.php#L48
https://github.com/getkirby/kirby/blob/4.3.0/src/Template/Template.php#L103

@afbora
Copy link
Member

afbora commented Jul 3, 2024

As workaround for users, you can block all *.html requests via custom route if you need.

'routes' => [
    [
        // blocks all requests to *.html and returns 404
        'pattern' => '(:all)\.html',
        'action' => function ($all) {
            return false;
        }
    ]
]

afbora added a commit to getkirby/getkirby.com that referenced this issue Jul 3, 2024
@distantnative
Copy link
Member

distantnative commented Jul 13, 2024

I'm thinking this could be solved by

  • Template class having a public static property for the default type used by default (maybe $defaultType can still be passed as parameter to divert from the global default type, but maybe that's also unnecessary and we replace the parameter by the public static property).
  • Kirby automatically redirects any route involved with using Template and ending in the same extension as the template default type to the URL without the extension (likely in App::resolve()).
    • Maybe the public static property isn't even needed and we could stick to doing something like $extension === $page->template()->defaultType()

@lukasbestle
Copy link
Member Author

TBH I think there should not be a default type at all, at least not for the routes and finding the correct template file. The only thing the type needs to matter for is the response Content-Type header (if a template without content representation extension is used, Kirby needs to send text/html).

@distantnative
Copy link
Member

@lukasbestle But what's the best way forward? Getting fully rid of a default type sounds like it could be result in much larger breaking changes as the template class is quite fundamental.

Wouldn't we solve this issue as well with checking for $extension === $page->template()->defaultType() and redirecting to the URL without extension? This way avoiding breaking changes deeper down in the stack?

@lukasbestle
Copy link
Member Author

The issue is: If a template.html.php exists, appending .html should work and render that template. By comparing the extension to the default extension, we would break that.

The template.php without representation extension should be used when the URL does not contain one. And exactly then.

So there are two cases IMO:

  • URL contains extension: Look for representation template. If none exists, render error page.
  • URL does not contain extension: Use base template.

I can't see a case that leaves room for a default extension and I have no idea why we (probably I) implemented it this way back then.

@distantnative
Copy link
Member

@lukasbestle I tried to implement it but this leads into a rabbit whole as in so many places Kirby has the premise that $contentType = 'html' is equivalent to no extension added (core component, controllers, representations, ...). I don't think it's right to rewrite this all.

In Kirby's logic, having two templates template.php and template.html.php is a logical error as template.php is already defined as the HTML template by Kirby's nature. So in Kirby, there should never be the need and case to append .html as one cannot have two different things serving to the HTML content type (with and without appendix).

Which is why I think .html content representations should redirect to the URL without this appendix.

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

No branches or pull requests

3 participants