getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.31k stars 168 forks source link

Controller not passing data to content representation if php template is not present #2471

Closed jmheretik closed 4 years ago

jmheretik commented 4 years ago

Describe the bug
I use Kirby as a headless CMS and am not interested in using the normal templates, only their content representation version, e.g. json. But the controller doesnt pass data to the content representation version of a template if its normal template is not present as well.

To Reproduce
Steps to reproduce the behavior:

  1. Go to site/templates and remove all templates, leave only site/templates/default.php
  2. Add a json content representation template for some page that exists and uses this template, e.g. site/templates/album.json.php (in starterkit)
  3. Add a controller for this representation, i.e. site/controllers/album.json.php and try to pass some variable to the json template.
  4. Doesnt work. But if you create a file for the normal template, i.e. site/templates/album.phpand even leave it empty, suddenly the variable from the controller gets passed to the json template.

Expected behavior
The controller for the json template passes data to the json template even without having to have a normal (non-json) template present.

Kirby Version
3.3.4

lukasbestle commented 4 years ago

I'd say that this is intended behavior or an unsupported case: A content representation is just one representation of a specific template. What would happen if only a .json template is defined but the page is accessed without a representation at all or with .xml? As there wouldn't be a template to fall back to, Kirby would need to fall back to the default template, which is even more off from the desired behavior than just a wrong representation of the correct template.

@jmheretik For your use-case I'd recommend just using simple templates and not the content representation feature, which is (as I explained) only useful if you need multiple representations. If you just need JSON output, you can print JSON from the main template. Setting the response content type is possible with kirby()->response()->type('json');.


What is actually the bug in my opinion is that content representation templates without the base template kind of work, but not fully. I think that Kirby should fall back to the default template and its content representations even if the currently accessed representation would have a perfectly matching template. That's not optimal either, but otherwise behavior is pretty inconsistent – an unsupported case should rather be fully unsupported. What do you think @afbora @distantnative?

afbora commented 4 years ago

@lukasbestle In fact i don't think we have changed the current process in representation requests right now. It was already working that way, but it was missing :thinking: So we didn't change fallback process.

https://github.com/getkirby/kirby/blob/3.3.4/src/Cms/Page.php#L1133-L1137 https://github.com/getkirby/kirby/blob/3.3.4/src/Cms/Page.php#L1168-L1179

lukasbestle commented 4 years ago

The issue is: I think that Page::representation() should not only check if the representation template exists, but also if the base template exists. Probably the most reliable way would be just changing this line from intendedTemplate() to template(). The representation to use will then always be based on the base template availability.

afbora commented 4 years ago

Hmm, what if we check the default template in second order?

For example:

public function representation($type)
{
    $kirby          = $this->kirby();
    $template       = $this->intendedTemplate();
    $representation = $kirby->template($template->name(), $type);

    if ($representation->exists() === true) {
        return $this->template = $representation;
    }

    // fallback to default template
    $defaultTemplate = $this->template();
    if ($defaultTemplate->exists() === true) {
        return $this->template = $kirby->template($defaultTemplate->name(), $type);
    }

    throw new NotFoundException('The content representation cannot be found');
}
afbora commented 4 years ago

@lukasbestle In the representation with the last comment, the template that will not be used (/site/templates/album.php) will not be mandatory and fallback will continue to work as we wish in the absence of the relevant template file.

lukasbestle commented 4 years ago

I think there's a misunderstanding: The issue I'm describing that we should fix is the following:

Imagine a site with the following templates in site/templates:

default.php
default.json.php
project.json.php

Note that the project.php template is missing.

The behavior should now be the following:

afbora commented 4 years ago

@lukasbestle I clearly couldn't understand why the template ordering was not as follows:

Request on /project-page.json:

  1. project.json.php
  2. default.json.php
  3. The content representation cannot be found

Is this technically difficult or is there a logical issue in process? Because the project.php template file has no place in the presentation process. So why are we dependent on project.php file?

lukasbestle commented 4 years ago

Is this technically difficult or is there a logical issue in process? Because the project.php template file has no place in the presentation process. So why are we dependent on project.php file?

@afbora Because if the main template (project.php) is missing, the results will be inconsistent between different representations:

If you request /project-page or /project-page.xml, you will get the main template of the default template, but if you request /project-page.json, you would suddenly get the representation template of the project template. Such an inconsistency (especially when paired with a controller and/or model) is very hard to debug, which is why the whole project template should be treated as if it wouldn't exist, including the project.json.php representation that does exist. Basically the project.json.php template should be completely ignored in this case.

This can be done by changing the one line from intendedTemplate() to template().

bastianallgeier commented 4 years ago