search

getkirby / kirby

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

Updating a page on the fly results in undefined variable page #2555

Closed mrflix closed 3 years ago

mrflix commented 4 years ago

Describe the bug
When I update a page on the fly, snippets will report page and site as undefined.

To Reproduce
Steps to reproduce the behavior: Controller (simplified)

return function($site, $page, $kirby) {
  if($kirby->request()->is('post') === true){
    $kirby->impersonate('kirby');
    $participants = $page->participants()->yaml();

    $participants[] = [
      'name' => get('name'),
      'email' => get('email'),
    ];

    $page = $page->update([
      'participants' => $participants
    ]);
  }
};

Template:

snippet('header');

Snippet header:

<!DOCTYPE html>
<html data-template="<?= $page->template() ?>">

Expected behavior
The page updates itself on the fly and renders correctly.

Screenshots
Google Chrome 2020-04-03 at 11 56 01

Kirby Version
3.35

afbora commented 4 years ago

It shouldn't throw undefined variable error, but I think we are overwriting even if the page data returns from the controller 🤔

https://github.com/getkirby/kirby/blob/3.3.5/src/Cms/Page.php#L347

lukasbestle commented 4 years ago

@mrflix Hm, I cannot reproduce this with a reduced test case:

site/controllers/home.php:

<?php

return function($kirby, $page) {
    $kirby->impersonate('kirby');
    $page = $page->update(['test' => uniqid()]);
};

site/templates/home.php:

Template: <?= $page->test() ?><br>
<?php snippet('test') ?>

site/snippets/test.php:

Snippet: <?= $page->test() ?>

Output:

Template: 5e88aa98978ec
Snippet: 5e88aa98978ec
afbora commented 4 years ago

@lukasbestle If you try like following, you'll see the issue:

return function($kirby, $page) {
    $kirby->impersonate('kirby');
    echo "Controller: ". $uniqid = uniqid();
    $page = $page->update(['test' => $uniqid]);
};
lukasbestle commented 4 years ago

@afbora Nope:

Controller: 5e88ad49f3869Template: 5e88ad49c9d78
Snippet: 5e88ad49c9d78
afbora commented 4 years ago

@lukasbestle All variables should be 5e88ad49f3869. But you'll only get 5e88ad49f3869 variable in next request for templates.

lukasbestle commented 4 years ago

But that's just because the $page object is not changed, only a clone of it, which is to be expected with the current implementation. But this is unrelated to the original issue @mrflix has reported ("undefined variable").

afbora commented 4 years ago

Yes, shouldn't give such a error. I couldn't reproduce the issue too. But shouldn't I be able to send the $page object I updated in the controller to the template like following?

<?php

return function($kirby, $page) {
    $kirby->impersonate('kirby');
    $page = $page->update(['test' => uniqid()]);

    return [
        'page' => $page
    ];
};
lukasbestle commented 4 years ago

But that's an entirely different problem... I have created an issue here: https://github.com/getkirby/kirby/issues/2559

mrflix commented 4 years ago

Lukas and Ahmet, thanks for looking into this. After a lot of debugging I was able to boil down the error: when I turn off specific numbering in the blueprint (num), the error disappears. When num is active, $__data in Tpl->load or I think more specific the whole $kirby->data object turns into an empty array, breaking$page, $site etc.

I used this num:

num: '{{ page.date.toDate("%Y%m%d") }}'

I was able to follow the error until here in $page->createNum https://github.com/getkirby/kirby/blob/6bd14bc8099d6c9dbe67ef0582b8c0a2d4c0888c/src/Cms/PageActions.php#L514-L525

It's getting called in $page->update when num is active. The full code of createNum: https://github.com/getkirby/kirby/blob/6bd14bc8099d6c9dbe67ef0582b8c0a2d4c0888c/src/Cms/PageActions.php#L474-L527

This taught me, that I can just use date. date and datetime should be mentioned here in the docs. After switching to num: 'data' the update error is gone. This leads me to the assumption, that the error stems from the last switch statement in createNum.

lukasbestle commented 4 years ago

@mrflix Thanks for debugging this further. That's all incredibly weird. The only thing that could cause this that I can think of is the clone() operation on the App object, but even then I don't know why it would. I will do some more tests.

lukasbestle commented 4 years ago

@mrflix Unfortunately I can still not reproduce this. Could you please test this with a fresh Plainkit and write down the exact steps needed to reproduce the issue? The more minimal the testing setup is the better for debugging. Thanks in advance!

distantnative commented 4 years ago

Info from the other (duplicate) thread: Reportedly happens when using a date as num in the non-default language.

Removing ->clone() fixes it but will result in other problems.

nilshoerrmann commented 4 years ago

@distantnative: The issue is not bound to multilingual setups. In my case it‘s a single language install but with a date field not named date (thus requiring a query for the num sorting).

nilshoerrmann commented 4 years ago

@lukasbestle: You said that you couldn't reproduce the issue as described in this thread. Are you able to reproduce following #2737?

lukasbestle commented 4 years ago

Yes, with your step-by-step explanation I can reproduce it. But weirdly only in the Starterkit, not in the Plainkit. That's all so weird. 🤯

shoesforindustry commented 4 years ago

Indeed I may have something similar, if not actually the same. I thought I had it fixed but the 3.4.0 release (same on latest beta) seems to make it not work again.
See Changing status from draft to listed in panel, does not seem to update json file in a hook? for info:

lukasbestle commented 4 years ago

Thanks, I will debug this some more when I find the time.

afbora commented 4 years ago

I think found the source of issue.

When the App class is cloned, we always overwrite the static instance. I think this is a serious problem. Doesn't it have to be separate from its instance or use same instance when we clone it?

I have tested. If you don't update static::$instance or separate when cloned, it works correctly as expected.

https://github.com/getkirby/kirby/blob/3.4.2/src/Cms/App.php#L113

afbora commented 4 years ago

So the source of this error is not actually a page update on fly. It's about the Kirby clone. Another example;

/site/controllers/note.php

A simple kirby cloning line like the one below will cause you to get the error.

return function($kirby) {
    $kirby->clone();
};
afbora commented 4 years ago

@lukasbestle I found some clearer details of the issue. What I will write is not a suggestion, but only to point out the source of the issue.

  1. When the controllers are called, $kirby->data is not set yet. The data property of App class must be set before the controller is called. It can use $kirby->data in the controller. As in the $page->update() example.
$data = array_merge($data, [
    'kirby' => $kirby = $this->kirby(),
    'site'  => $site  = $this->site(),
    'pages' => $site->children(),
    'page'  => $site->visit($this)
]);

// set public data before controller run
$kirby->data = $data;

// call the template controller if there's one.
$controllerData = $kirby->controller($this->template()->name(), $data, $contentType);

https://github.com/getkirby/kirby/blob/3.4.2/src/Cms/Page.php#L340-L348

  1. When the App class is cloned, we transfer all the values of the old cloned object to the new one, but we do not pass the data property. Because we overwrite the static instance and the data property values are lost, it remains on the old object.
shoesforindustry commented 4 years ago

This does not appear to be fixed in the new version 3.4.3?

afbora commented 4 years ago

@shoesforindustry Not yet, @lukasbestle working on it. I hope this issue will fixed on next release.

shoesforindustry commented 4 years ago

Thanks afbora, happy to do some testing if required...

lukasbestle commented 4 years ago

Related issue: https://github.com/getkirby/kirby/issues/2581

andreasotto commented 4 years ago

+1 Some problem here.

Updating a page from it's controller (just adding something to a field) results in the error Undefined variable: site. The blueprint is using:

num: "{{ page.from.toDate('Ymd') }}"

and I can't do without it. As soon as i comment that num: line in the blueprint out, everything is ok.

I've got a site where i heavily rely on this. Is there any workaround for the $page->update() in the controller as long as this bug isn't fixed?

afbora commented 4 years ago

Is there any workaround for the $page->update() in the controller as long as this bug isn't fixed?

@andreasotto For single or multi-lang website?

andreasotto commented 4 years ago

single

afbora commented 4 years ago

As workaround, you can remove the ->clone() method from following line for only non-multilingual website:

https://github.com/getkirby/kirby/blob/3.4.3/src/Cms/PageActions.php#L548

$app = $this->kirby();
$app->setCurrentLanguage();
shoesforindustry commented 4 years ago

Does not seem to be fixed in 3.4.4? Any updates?

lukasbestle commented 4 years ago

@shoesforindustry Sorry for the delay. I have not forgotten it and will take a look at it soon.

shoesforindustry commented 4 years ago

No problem @lukasbestle consider it a very gentle reminder. I did try the fix above from @afbora with a single language site and it seems to work, but of course we also require it on multi-lingual site.

bastianallgeier commented 3 years ago

✅