Infinite loop, caused by changing the method_exists call on the lumberjack extension

[erikfrerejean]

#125 introduces a breaking change

Because the new $this->getOwner()->hasMethod('getLumberjackPagesForGridfield') checks whether the method is defined anywhere on the owner or any of its extensions it will always return true as the \SilverStripe\Lumberjack\Model\Lumberjack extension itself defines the getLumberjackPagesForGridfield method.

In the old case when an implementation didn't define the getLumberjackPagesForGridfield method itself but used the Lumberjack extension it would fallback to the default behaviour of loading all sitetree items with the current owner as parent id.

After the change in #125, when the implementation itself doesn't explicitly define the getLumberjackPagesForGridfield method the current codeflow will call itself and cause an infinite loop. We've validated this behavior by replacing the method with the following workaround which brought back the behaviour previously observed.

    public function getLumberjackPagesForGridfield($excluded = array())
    {
        static $incall = false;
        if ($this->getOwner()->hasMethod('getLumberjackPagesForGridfield') && !$incall) {
            $incall = true;
            $field = $this->owner->getLumberjackPagesForGridfield($excluded);
            $incall = false;
            return $field;
        }

        return SiteTree::get()->filter([
            'ParentID' => $this->owner->ID,
            'ClassName' => $excluded,
        ]);
    }

PRs

• FIX Remove infinite loop by reverting #127 #129

[erikfrerejean]

To ealisy replicate this issue you can install 4.1.3 of our articles module https://github.com/wedevelopnl/silverstripe-articles with the current version of lumberjack. When adding an articles page you'll get a service unavailable. After reverting lumberjack the module works as expected.

We've patched out the bug in module version 4.1.4, but this will most likely break in a lot more instances.

[GuySartorelli]

Thanks for reporting this.

@wernerkrauss
Can you see a way forward that retains has_method() (or rather retains the ability to introduce the method through an extension other than this one?) If not we'll have to simply revert your recent change.

[wernerkrauss]

Ah, that's a pity. I didn't see, that the names are the same and cause this problems.
I don't know a way to easily check, if the method exists in the owner class or any other extension.
Maybe introducing a different hook to update the config would be better.

[emteknetnz]

I've merged the revertion PR and tagged as 2.3.3

@wernerkrauss yes introducting a new hook such as updateLumberjackGridFieldConfig seems pretty reasonable. If you do raise a PR for that could you also add them for the other get* methods in the file and add notes explaining why we need them i.e. because the method_exists() approach only works for methods added on the Page class and not extensions

[erikfrerejean]

Thanks for the fix all :)

[GuySartorelli]

No worries, thanks for reporting it @erikfrerejean

Closing, as the issue is resolved.

@wernerkrauss if you want to follow up on the comment above from @emteknetnz please comment about it in your original issue #127 and we can re-ooen that issue for you.