multiple updates in hooks override one another

[moritzebeling]

Describe the bug
When having multiple plugins using the same hook to update() the $file or $page object, only one change actually comes through, the other gets lost or is overwritten.

To Reproduce

1. Install 2 plugins that both use the file.create:after hook (or similar) to initially fill one field. e.g.:

Kirby::plugin('kirby/plugin-1', [
  'hooks' => [
    'file.create:after' => function ($file) {
      $file->update([
        'title' => 'Title'
      ]);
    },
  ]
]);
Kirby::plugin('kirby/plugin-2', [
  'hooks' => [
    'file.create:after' => function ($file) {
      $file->update([
        'caption' => 'Caption'
      ]);
    }
  ]
]);

2. Upload an image
3. Check for the content file, only one of both fields got filled in

Expected behavior
Would be nice, if both fields would be filled in.

Kirby Version
3.4.3

Forum discussion https://forum.getkirby.com/t/how-to-trigger-file-update-in-two-plugins-that-use-the-same-hook/19072/5

[afbora]

Kirby runs the hooks in turns. The first hook is working and updating properly (you can test with sleep function in first hook). When it comes to the second one, the second hook does not know about the updates on the first hook, as there is still the old object in the cache. Thus, when the second hook is completed, it overwrites the changes made by the first hook.

[lukasbestle]

Another situation where our immutable objects unfortunately cause issues. :( Without global state in memory it's likely going to be hard to fix this.

[afbora]

The number of issues with immutable objects is growing. I hope we find a general solution.

[crisgraphics]

I'm also having trouble with this more often now. It makes the usage of quite a handful plugin combinations impossible :-/

[GiantCrocodile]

It would be great if we find a solution for this. My Kirby 3 projects are idling/far away from being finished or productive deployed but this is one scenario I see quite common and likely to happen in multiple projects/use cases so it would be great if we find some solution for Kirby 3 rather than Kirby 4. I think it's even quite common to think about this even when you just heard of Kirby3/it's hooks or plug-in system without using it at all.

[moritzebeling]

Hey hey, is there any update on this? A solution in sight? Or a workaround or hack?

In my scenario I have 3 plugins all using the file.create:after hook. 2 of them I created especially for that very project, so I could merge them into one plugin and only use one single hook. But the 3rd plugin is the color extractor. I would prefer to not take it apart... But maybe that's the only way...

[afbora]

@moritzebeling Actually, the issue stems from using update inside the create hook. It wouldn't be a problem if both are .update hooks. As a workaround, you can use the first as .create and the second as .update hooks. The update() method on the first hook will trigger the second hook.

// listen the create hook and trigger the update hook
Kirby::plugin('kirby/plugin-1', [
  'hooks' => [
    'file.create:after' => function ($file) {
      $file->update([
        'title' => 'Title'
      ]);
    },
  ]
]);

// listen the update hook 
Kirby::plugin('kirby/plugin-2', [
  'hooks' => [
    'file.update:after' => function ($newFile, $oldFile) {
      $newFile->update([
        'caption' => 'Caption'
      ]);
    }
  ]
]);

// listen the update hook 
Kirby::plugin('kirby/plugin-3', [
  'hooks' => [
    'file.update:after' => function ($newFile, $oldFile) {
      $newFile->update([
        'alt' => 'Alt'
      ]);
    }
  ]
]);

[rasteiner]

Here's a braindump.

We could work around this particular issue by enabling plugin devs to explicitly specify which hook arguments they want to modify, by allowing an API like this:

Kirby::plugin('kirby/plugin-1', [
  'hooks' => [
    'file.create:after' => function (&$file) {
      $file = $file->update([
        'title' => 'Title'
      ]);
    },
  ]
]);

We should be able to do this by editing 2 classes: Kirby\Toolkit\Controller and Kirby\Cms\Event. Like this:

Event.php

    public function call(?object $bind, Closure $hook)
    {
        // collect the list of possible hook arguments
        $data = $this->arguments();
        $data['event'] = $this;

        // magically call the hook with the arguments it requested
        $hook = new Controller($hook);

        //let the Controller class edit the data list
        $ret = $hook->call($bind, $data); 
        $this->arguments = $data; 

        return $ret;
    }

Controller.php

    public function arguments(array $data = [], array &$map = null): array
    {
        $info = new ReflectionFunction($this->function);
        $args = [];

        foreach ($info->getParameters() as $parameter) {
            $name = $parameter->getName();
            // if hook wants an argument by Reference, pass it by reference
            if ($parameter->isPassedByReference()) {
                $args[] = &$data[$name] ?? null;
            } else {
                $args[] = $data[$name] ?? null;
            }

            //which argument goes where
            if($map !== null) {
                $map[$name] = $parameter->getPosition();
            }
        }

        return $args;
    }

    public function call($bind = null, &$data = [])
    {
        $map = [];
        $args = $this->arguments($data, $map);

        if ($bind === null) {
            $ret = call_user_func_array($this->function, $args);
        } else {
            $bound = $this->function->bindTo($bind);
            $ret = call_user_func_array($bound, $args);
        }

        //update the arguments
        foreach ($map as $name => $position) {
            $data[$name] = $args[$position];
        }

        return $ret;
    }

Should be backwards compatible with the current api, but haven't tested it.

[lukasbestle]

I like this solution a lot! This would work quite well assuming we want hooks to be able to overwrite their arguments (not fully sure about security and reliability implications yet, but so far I agree that this is more useful than harmful).

[rasteiner]

Don't know about security (I'd presume that allowing this isn't any riskier for security than allowing any plugin to execute in general - I mean, in the worst case a plugin could already now alter the Kirby sourcecode and apply the above changes :3). The biggest problem I see would be "debugging": if Plugin A does something stupid like $page = 'blah'; the error would surface only when Plugin B tries to use the page. Otherwise the modified variable ceases to exist once the event has finished triggering; that is: it shouldn't propagate to the scope in PageActions or stuff like that...

There are only 3 ways to get data out of a function:

1. the return value
2. pass by reference variables
3. the function has side effects

Number 3 isn't very compatible with immutability. The best I could imagine would be to have a separate "Storage Service" that remembers "updates" and then commits all of them together (like at the very end of a request), otherwise having a central storage would make it extremely complicated to have functionality like the current "oldPage" and "newPage" in update hooks.

Number 2 I think requires the least change (that's why I wrote it).

Number 1 (return values) could be done in 2 ways: The first would be doing what gets done with the kirbytext hooks, and is probably the most "expected" one by plugin devs. Maybe changing the PageActions and FileActions commit function to make it use apply instead of trigger would be enough for this issue? Of course this requires some more specific code (the commit function would have to know which parameter needs updating, that could be simplified with some heuristics (like, PageActions normally would update "page", FileActions would update "file", otherwise it seems to be always the "first" parameter that could be passed to apply as $modify string). The second could be something like

//myhook 
function($page) {
  return ['page' => $page->update([ 'title' => 'title' ]) ];
};

PS: if I get this correctly, such a change would allow you to deprecate either the apply or trigger function.

[lukasbestle]

I feel like passing arguments by reference is more of an advanced feature as it's weird for devs who are not 100% familiar with memory management. Simply returning a value makes a lot of sense for filter-type hooks (e.g. kirbytext, route:after), so I think we should still keep both apply() and trigger().

But for use cases like this (updating Kirby models), requesting an argument by reference is a really simple and clean solution.

[distantnative]

@lukasbestle this one would be ideal for a summary - it feels like you all were already quite close to a concept, but breaking the last state down would be great.

[lukasbestle]

TBH I have the feeling that we should get rid of the immutable objects in the long term. We've had all sorts of issues, e.g. also https://github.com/getkirby/kirby/issues/2767.

Moving back to mutable objects would reduce the overall complexity by a lot. With just one object in memory for each model, we will have a global state that always matches what's on the disk. @rasteiner's solution is really good when the objects are immutable. But if we make them mutable again, this solution is not needed and users don't need to think about requesting objects by reference.

The migration towards mutable objects isn't going to be easy though. E.g. we would still need to provide a way to clone objects with all their properties and child objects to "freeze them in time". This is needed for hooks so that we can provide e.g. $oldPage. Cloned objects should then have a marker that prevents them from being used as the base of additional changes (e.g. calling $oldPage->update() would throw an exception).

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. This is for us to prioritize issues that are still relevant to our community. It will be closed if no further activity occurs within the next 14 days. If this issue is still relevant to you, please leave a comment.

[mrflix]

I think I just also ran into this issue: I have a hook on file.update:after to create a Kirby\Cms\Block with the uploaded file. When I upload multiple files, the update calls inside the hook turn into Russian roulette: they overwrite each so some changes get stored, some don't.

^{4.0.0-alpha.2}

Edit: Since the goal is to use every file in the page in a block (each block is a slide containing one or more photos/videos) I for now rewrote the hook to add all unused files as blocks every time the hook gets called. Hacky but works for now. Edit#2: nvm, doesn't work: the $page->files() in each hook call doesn't always contain all the new files.