[Bug]: Inconsistent Inherited Values Setter

[solverat]

Expected behavior

Solid Runtime definitions for static variables (Like $getInheritedValues)

Actual behavior

Inconsistent state definitions.

Steps to reproduce

PIMCOREs usage of DataObject::setGetInheritedValues(false); is a very dangerous thing. Using the messenger component within a supervisor process, for example, this became even more dangerous.

We had a very strange issue within a large project and I had a hilarious time, debugging it.

In our case, the messenger queue is working asynchronously in multiple cores. Within that process, we randomly fetched a custom exception in our code, which told us, that an inherited value is not available. But that was not the case - the value should be there.

So, what happened?

Here, you're going to disable the inheritance feature:

https://github.com/pimcore/pimcore/blob/0520fd4e7b0e8cf33de43625c09f1f62612096f4/models/DataObject/Localizedfield/Dao.php#L170

Here, an exception has been thrown (in our case it was a deadlock exception because of the multicore process):

https://github.com/pimcore/pimcore/blob/0520fd4e7b0e8cf33de43625c09f1f62612096f4/models/DataObject/Localizedfield/Dao.php#L314

So it will never reach the point to restore the inheritance state:

https://github.com/pimcore/pimcore/blob/0520fd4e7b0e8cf33de43625c09f1f62612096f4/models/DataObject/Localizedfield/Dao.php#L443

---

We caught the exception and moved on. BUT! That's evil, because from now on, the inheritance feature is disabled "forever".

I guess there are more "dangerous" places within the pimcore core and I would suggest moving all the logic into a wrapper. Getting rid of these static "switches" would be awesome, but also a heavy task, I guess.

In my private code, I'm using a snippet like this:

<?php

class InheritanceHelper
{
    public static function useInheritedValues(\Closure $function, bool $inheritValues = true)
    {
        $backup = DataObject\AbstractObject::getGetInheritedValues();
        DataObject\AbstractObject::setGetInheritedValues($inheritValues);

        $error = null;

        try {
            $result = $function();
        } catch (\Throwable $e) {
            $error = $e;
        }

        DataObject\AbstractObject::setGetInheritedValues($backup);

        if ($error instanceof \Throwable) {
            throw $error;
        }

        return $result;
    }
}

[jdreesen]

I agree, that the current solution is dangerous (and tedious to write). I use something similar:

class InheritanceHelper
{
    public static function useInheritedValues(bool $inheritValues, callable $fn, array $args)
    {
        $backup = DataObject::getGetInheritedValues();
        DataObject::setGetInheritedValues($inheritValues);

        try {
            return $fn(...$args);
        } finally {
            DataObject::setGetInheritedValues($backup);
        }
    }
}

[brusch]

I agree as well, the current solution is not very safe and also strange to use. I also agree with the fact that this one is very hard to change 😊

But I don't think we should just accept that status quo, so I'd like to invite you to make suggestions how a better approach could look like and discuss and validate them together.

Looking forward to your proposals.

[jdreesen]

Well, using one of those snippets would be an improvement ;)

[brusch]

I'm not sure if that would work in all cases, but yes, that would be a good start. I think we could just add this small snippet to the existing InheritanceHelper and then replace the legacy code bit by bit, WDYT?

[brusch]

@jdreesen @solverat anyone wants to create a PR for the helper incl. docs? Thanks! 😊

[jdreesen]

I think I can do that on Friday (if @solverat isn't faster than me :wink:)

[jdreesen]

It's actually way more to do than I expected, if you want to apply this to every case, so I need some more time for this ;)

[brusch]

👍 sure, looking forward to the PR 😊

[jdreesen]

I'm sorry, but it seems like I won't have time to work on this anytime soon :/ I've been very busy in the last weeks and will be on vacation in August.

Maybe @solverat will give it a try?

[brusch]

@jdreesen @solverat any chance you can provide us a PR for that? 😊

[jdreesen]

I'd love to, but my time is short at the moment :disappointed:

[mcop1]

While looking through another issue I noticed that there is already something like that (at least for the demos): https://github.com/pimcore/demo/blob/fca7d5ace4ba318267e37c19e72384bb4b422135/src/Website/Tool/ForceInheritance.php#L28 Can be removed after we implemented the functionality above in the core.