getkirby / kirby

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

[K4] Session: need to serialize data. Regression? #6061

Closed vauvarin closed 9 months ago

vauvarin commented 9 months ago

Description

When I set a value in Session which is not serialize, I get an error:

Uncaught Exception: Serialization of ‘Closure’ is not allowed in /home/…/kirby/src/Session/Session.php:372

I didn't get this error with Kirby 3.*

To reproduce

Run this code in a controllers or templates:

$session = $kirby->session(); $session->set( 'title', $page->title() ); $session->set( 'pages', $site->children()->unlisted() );

Your setup

Kirby 4.0.1

bastianallgeier commented 9 months ago

Wow, did this code really work in v3? That's interesting. @lukasbestle do you have any idea what we might have changed to break this – or better question, how is it even possible that it worked before?

lukasbestle commented 9 months ago

Did we introduce a closure value somewhere in the props of one of the involved objects? If so, we'd need to implement __serialize() or __sleep().

afbora commented 9 months ago

Yes, works in K3.

afbora commented 9 months ago

I couldn't find source of the issue yet. But I'm pretty sure that this is not related with session namespace. serialize($page) works for K3 but not in K4.

lukasbestle commented 9 months ago

Just tried inspecting the $page object and its object tree for closures, but couldn't spot any obvious ones except the roots, URLs, field methods, routes etc. that were all already in v3 and that also shouldn't be serialized.

We probably need to start searching in smaller objects that don't pull in the entire core. 😅

afbora commented 9 months ago

You can also reproduce with serialize($page->title()) easily

bastianallgeier commented 9 months ago

Just another thought. Could this be related to type hints somehow? It's really a stupid idea, but maybe the serializer didn't bother to serialize something before without type hints.

lukasbestle commented 9 months ago

AFAIK, serialization just takes whatever is actually in memory and converts it to a string representation. Type hints don't affect the memory content directly, but you never know which cascade of behavior may cause this.

lukasbestle commented 9 months ago

Just found this tool that could help us here: https://github.com/EdeMeijer/serialize-debugger (If it still works with PHP 8.1)

afbora commented 9 months ago

Wow, it seems works great. But there are multiple errors for $page->title(). I'm not quite understand the issue 🤷‍♂️

Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][kirby]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][kirby]
Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][i18n]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][i18n]
Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][i18n:translations]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][i18n:translations]
Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][i18n:rules]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][i18n:rules]
Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][index]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][index]
Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][assets]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][assets]
Closure - ERROR
    {root}->parent->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->site->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->propertyData[site]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->site->children->data[photography]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->site->children->data[notes]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->site->children->data[about]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->site->children->data[error]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->site->children->data[sandbox]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->propertyData[site]->children->data[photography]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->propertyData[site]->children->data[notes]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->propertyData[site]->children->data[about]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->propertyData[site]->children->data[error]->propertyData[kirby]->core->cache[roots][content]
    {root}->parent->propertyData[site]->children->data[sandbox]->propertyData[kirby]->core->cache[roots][content]
    ...
    ...
    ...
afbora commented 9 months ago

Here serialize-debugger plugin for kirby: serialize-debugger.zip

You can use like following in template or controller after install the plugin:

\EdeMeijer\SerializeDebugger\Debugger::debugHTML($page->title());
rasteiner commented 9 months ago

Since I've pointed vauvarin to Github I feel a bit involved.

Here's a "in house" script that maybe is a bit easier to understand (just because it's less than 60 lines of code).

<?php

require 'kirby/bootstrap.php';

use Kirby\Cms\App as Kirby;

$kirby = new Kirby();

function searchSerializationError($thing, string $path = '') {
    static $recursionCheck = new WeakMap();

    if(is_object($thing)) {
        if($recursionCheck->offsetExists($thing)) {
            return null;
        }

        $recursionCheck->offsetSet($thing, true);
    }

    try {
        serialize($thing);
    } catch (Exception $e) {
        if(is_callable($thing)) {
            return $path;
        } else {
            if(is_object($thing)) {
                $refl = new ReflectionObject($thing);
                foreach($refl->getProperties() as $prop) {
                    $prop->setAccessible(true);
                    // skip if static 
                    if($prop->isStatic()) {
                        continue;
                    }
                    // check if initialized
                    if(!$prop->isInitialized($thing)) {
                        continue;
                    }
                    $value = $prop->getValue($thing);
                    $result = searchSerializationError($value, $path . '->' . $prop->getName());
                    if($result) {
                        return $result;
                    }
                }
            } else if(is_array($thing)) {
                foreach($thing as $key => $value) {
                    $result = searchSerializationError($value, $path . '["' . $key . '"]');
                    if($result) {
                        return $result;
                    }
                }
            }
        }
    }

    return null;
}

echo searchSerializationError(page('home')->title(), 'page("home")->title()') . "\n";

The output however points to the same path as the other tool (it just does so only once):

page("home")->title()->parent->site->propertyData["kirby"]->core->cache["roots"]["kirby"]

So, essentially Site stores a pointer to the kirby instance as instance member (in the $propertyData array). Kirby (class App) has a Core $core property, which has a $cache array, which contains a 'roots' array which has a closure as the 'kirby' offset.

In other words, implementing Serializable in strategically chosen classes would be the best, since there's not really a point in Serializing the complete website when the intent is to serialize a single field or a collection of pages.

lukasbestle commented 9 months ago

Could this be related to our Properties refactoring? For some reason the property data wasn't serialized in v3 but now is serialized in v4 and that really doesn't make much sense.

rasteiner commented 9 months ago

Serializing page('home')->title() in a 3.9.8 plainkit gets you:

O:15:"Kirby\Cms\Field":3:{
  s:6:"*key";s:5:"title";
  s:9:"*parent";O:14:"Kirby\Cms\Page":22:{
    s:7:"*site";O:14:"Kirby\Cms\Site":17:{
      s:7:"*site";N;
      s:15:"*propertyData";a:11:{
        s:4:"site";N;
        s:7:"content";N;
        s:12:"translations";N;
        s:9:"blueprint";N;
        s:11:"errorPageId";s:5:"error";
        s:10:"homePageId";s:4:"home";
        s:4:"page";N;
        s:3:"url";s:1:"/";
        s:8:"children";N;
        s:6:"drafts";N;
        s:5:"files";N;
      }
      s:7:"content";N;
      s:12:"translations";N;
      s:12:"*blueprint";N;
      s:12:"*errorPage";N;
      s:14:"*errorPageId";s:5:"error";
      s:11:"*homePage";N;
      s:13:"*homePageId";s:4:"home";
      s:12:"*inventory";a:3:{
        s:8:"children";a:2:{
          i:0;a:5:{
            s:7:"dirname";s:5:"error";
            s:5:"model";N;
            s:3:"num";N;
            s:4:"root";s:39:"/398/content/error";
            s:4:"slug";s:5:"error";
          }
          i:1;a:5:{
            s:7:"dirname";s:4:"home";
            s:5:"model";N;
            s:3:"num";N;
            s:4:"root";s:38:"/398/content/home";
            s:4:"slug";s:4:"home";
          }
        }
        s:5:"files";a:0:{}
        s:8:"template";s:4:"site";
      }
      s:7:"*page";N;
      s:7:"*root";s:33:"/398/content";
      s:6:"*url";s:1:"/";
      s:8:"children";O:15:"Kirby\Cms\Pages":6:{
        s:4:"data";a:2:{
          s:5:"error";O:14:"Kirby\Cms\Page":22:{
            s:7:"*site";r:4;
            s:15:"*propertyData";a:15:{
              s:4:"site";r:4;
              s:7:"content";N;
              s:12:"translations";N;
              s:9:"blueprint";N;
              s:7:"dirname";s:5:"error";
              s:7:"isDraft";b:0;
              s:3:"num";N;
              s:6:"parent";N;
              s:4:"root";s:39:"/398/content/error";
              s:4:"slug";s:5:"error";
              s:8:"template";N;
              s:3:"url";N;
              s:8:"children";N;
              s:6:"drafts";N;
              s:5:"files";N;
            }
            s:7:"content";N;
            s:12:"translations";N;
            s:12:"*blueprint";N;
            s:8:"*depth";N;
            s:10:"*dirname";s:5:"error";
            s:9:"*diruri";N;
            s:10:"*isDraft";b:0;
            s:5:"*id";s:5:"error";
            s:19:"*intendedTemplate";N;
            s:12:"*inventory";N;
            s:6:"*num";N;
            s:9:"*parent";N;
            s:7:"*root";s:39:"/398/content/error";
            s:7:"*slug";s:5:"error";
            s:11:"*template";N;
            s:6:"*url";N;
            s:8:"children";N;
            s:6:"drafts";N;
            s:17:"childrenAndDrafts";N;
            s:8:"*files";N;
          }
          s:4:"home";r:3;
        }
        s:16:"*caseSensitive";b:0;
        s:13:"*pagination";N;
        s:9:"*parent";r:4;
        s:8:"*index";N;
        s:18:"*indexWithDrafts";N;
      }
      s:6:"drafts";N;
      s:17:"childrenAndDrafts";N;
      s:8:"*files";N;
    }
    s:15:"*propertyData";a:15:{
      s:4:"site";r:4;
      s:7:"content";N;
      s:12:"translations";N;
      s:9:"blueprint";N;
      s:7:"dirname";s:4:"home";
      s:7:"isDraft";b:0;
      s:3:"num";N;
      s:6:"parent";N;
      s:4:"root";s:38:"/398/content/home";
      s:4:"slug";s:4:"home";
      s:8:"template";N;
      s:3:"url";N;
      s:8:"children";N;
      s:6:"drafts";N;
      s:5:"files";N;
    }
    s:7:"content";O:17:"Kirby\Cms\Content":3:{
      s:7:"*data";a:1:{
        s:5:"title";s:4:"Home";
      }
      s:9:"*fields";a:1:{
        s:5:"title";r:1;
      }
      s:9:"*parent";r:3;
    }
    s:12:"translations";N;
    s:12:"*blueprint";N;
    s:8:"*depth";N;
    s:10:"*dirname";s:4:"home";
    s:9:"*diruri";N;
    s:10:"*isDraft";b:0;
    s:5:"*id";s:4:"home";
    s:19:"*intendedTemplate";O:23:"Kirby\Template\Template":3:{
      s:14:"*defaultType";s:4:"html";
      s:7:"*name";s:4:"home";
      s:7:"*type";s:4:"html";
    }
    s:12:"*inventory";a:3:{
      s:8:"children";a:0:{}
      s:5:"files";a:0:{}
      s:8:"template";s:4:"home";
    }
    s:6:"*num";N;
    s:9:"*parent";N;
    s:7:"*root";s:38:"/398/content/home";
    s:7:"*slug";s:4:"home";
    s:11:"*template";N;
    s:6:"*url";N;
    s:8:"children";N;
    s:6:"drafts";N;
    s:17:"childrenAndDrafts";N;
    s:8:"*files";N;
  }
  s:5:"value";s:4:"Home";
}

The difference to v4 seems to be that propertyData in v3 didn't contain 'kirby'. Still, I'm pretty sure vauvarin didn't mean to put all this into the session when writing $session->set( 'title', $page->title() );

bastianallgeier commented 9 months ago

This is basically just revealing some place where the Kirby instance is passed as a prop to a page model. I don't know where yet. But as soon as I set

    public function __construct(array $props = [])
    {
        unset($props['kirby']);

In ModelWithContent, the objects can be serialized just fine.

bastianallgeier commented 9 months ago

I think I have a fix https://github.com/getkirby/kirby/pull/6072