Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Magic __get methods interfere with classes that share the same parent #10263

Closed
andrewandante opened this issue Mar 21, 2022 · 12 comments
Closed

Comments

@andrewandante
Copy link
Contributor

Affected Version

4.10 (is what I've tested on)

Description

I have in my project a Page and PageController (thanks, CMS)
I have two Page types:

  • CoolPage extends Page
  • DopePage extends Page

CoolPage has a $db field Summary
DopePage has a method getSummary()

Adding $Summary in my CoolPage.ss throws the error:

[Emergency] Uncaught BadMethodCallException: Object->__call(): the method 'getSummary' does not exist on CoolPage

I've tracked this down to the __get() method on ViewableData, because the following line returns true:

        if (strtolower($property) !== 'field' && $this->hasMethod($method = "get$property")) {

Which is because PageController::hasMethod('getSummary') returns true, as it's getting it from DopePage, but then it can't call it in the next line:

        return $this->$method();

because CoolPage, the current scope, doesn't.

I've been able to work around this by creating an otherwise empty CoolPageController extends PageController, but that feels inelegant.

@kinglozzer
Copy link
Member

Which is because PageController::hasMethod('getSummary') returns true, as it's getting it from DopePage

Is DopePage added into the mix via ->customise() or something? I’m not sure why PageController would ever be looking at DopePage when rendering a CoolPage

@andrewandante
Copy link
Contributor Author

That I'm not totally sure about to be honest, but it seems to come from:

    public function hasMethod($method)
    {
        return method_exists($this, $method) || $this->getExtraMethodConfig($method);
    }

calling (from Extensible):

    protected function defineMethods()
    {
        $this->defineMethodsCustom();

        // Define extension methods
        $this->defineExtensionMethods();
    }

And it comes in through defineExtensionMethods() - maybe this has to do with the magic connection between Page and PageController? In which case it's a CMS issue

@NightJar
Copy link
Contributor

This is really confusing to follow. Do you have some actual minimal reproduction code you could share instead? So far I read into it what Loz has stated already.

@andrewandante
Copy link
Contributor Author

Sure thing.

AndantePage.php

<?php

class AndantePage extends Page
{

    private static $db = [
        'Summary' => 'Varchar(255)',
    ];

}
NightjarPage.php

<?php

class NightjarPage extends Page
{

    public function getSummary()
    {
        return 'Nightjar in the day is just a Dayjar';
    }

}
AndantePage.ss

$Summary

Expected result when visiting any old AndantePage: The Summary field is rendered in the template.
Actual result: [Emergency] Uncaught BadMethodCallException: Object->__call(): the method 'getSummary' does not exist on AndantePage

@kinglozzer
Copy link
Member

I can’t recreate this - I just get nothing rendered for $Summary, or if I add something to that field in AndantePage in the CMS it renders as expected. NightjarPage then renders Nightjar in the day is just a Dayjar as expected too

@christopherdarling
Copy link
Contributor

Can't recreate this either on the latest release.
@andrewandante worth double checking you haven't overloaded __get() on page.php and/or have an extension applied to either page-type that's causing this?

@andrewandante
Copy link
Contributor Author

How curious. I'll dig in a little and see if it's coming from somewhere else, though I'm pretty sure we aren't doing anything particularly weird in this project. Will see what exactly I need to set up to reproduce.

@andrewandante
Copy link
Contributor Author

andrewandante commented Mar 22, 2022

Ah-ha. The AndantePage has to be the child of the NightjarPage

Plz excuse potato quality:

andantepage

Also worth noting that it doesn't happen the other way around - if AndantePage is the parent, it's fine.

@michalkleiner
Copy link
Contributor

If by the child you mean nested underneath, that could suggest it's related to the Hierarchy class or thereabouts.

@andrewandante
Copy link
Contributor Author

andrewandante commented Mar 23, 2022

nested underneath

Yes that's right. I suspect something to do with Scope and Hierarchy and the magic that lets Controllers pull methods from their Pages. More investigation required, but at least I can reproduce it and am not losing my mind entirely. Yet.

@kinglozzer
Copy link
Member

Had a quick look into this, I think the problem comes from the CustomMethods trait. During request processing of a parent/child page structure, a ContentController is constructed with NightjarPage, and then a second one with AndantePage. The problem is that the second one built “inherits” the custom methods defined by the first one, despite being an entirely fresh instance of the controller. This is because the list of custom methods is static, so shared across multiple instances:

/**
* Custom method sources
*
* @var array
*/
protected static $extra_methods = [];

After the controller is built for the parent page, it looks (roughly) like this:

$extra_methods = [
    ContentController::class => [
        'getSummary'
    ]
];

The second instance of ContentController comes along and inherits this, because it’s also an instance of ContentController. ViewableData does actually try to clear these methods when ->setFailover($page) is called, but it fails because it’s removing the child AndantePage’s methods instead of NightjarPage.

I’m not really sure what the best solution here is. Sharing these methods statically probably isn’t a good idea, but I’d be wary about performance regressions from changing that

@GuySartorelli
Copy link
Member

Oh hey, that looks like #11574
I'm gonna close this in favour of that one which strikes at the root cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants