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

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

Closed
jmheretik opened this issue Feb 24, 2020 · 9 comments
Assignees
Milestone

Comments

@jmheretik
Copy link

@jmheretik jmheretik commented Feb 24, 2020

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

afbora added a commit that referenced this issue Feb 25, 2020
@afbora afbora self-assigned this Feb 25, 2020
@afbora afbora linked a pull request that will close this issue Feb 25, 2020
4 of 4 tasks complete
@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Feb 25, 2020

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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Feb 25, 2020

@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 🤔 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

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Feb 25, 2020

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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Feb 25, 2020

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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Feb 26, 2020

@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

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Feb 27, 2020

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:

  • Request on /default-page: default.php
  • Request on /default-page.json: default.json.php
  • Request on /default-page.txt: Error ("The content representation cannot be found")
  • Request on /project-page: default.php
  • Request on /project-page.json: default.json.php
    (not project.json.php because that would be inconsistent with the case directly above)
  • Request on /project-page.txt: Error ("The content representation cannot be found")
@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Feb 29, 2020

@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

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Feb 29, 2020

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

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Mar 5, 2020

jmheretik added a commit to jmheretik/kirby-api-vue-starterkit that referenced this issue Mar 21, 2020
jmheretik added a commit to jmheretik/kirby-json-vue-starterkit that referenced this issue Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.