Silverstripe-framework: [3.3.1] allowed children doesn't work properly

[t3hn0]

$allowed_children is not respected anymore. If you select "add new page" above site structure in Pages, you get the list of all page types (except the ones which return false in canCreate()).

You have two scenarios:
1) If you select one of allowed pages it works.
2) If you select any other pages which should been disabled, you get an error:
Fatal error: Call to a member function forTemplate() on a non-object in */framework/admin/code/LeftAndMain.php on line 593

Btw, filter for allowed children works in context menu.

[tractorcow]

So you're saying children don't respect their parents anymore? I'm sure I've heard that elsewhere...

It looks like this shouldn't be too hard to test or debug. I'll try to find some time this week.

Have you tested this in 3.3.0, or 3.2 and found if the issue exists there as well?

[t3hn0]

Yes it doesn't respect parent.

I'll try reproducing error in 3.2 tomorrow.

[t3hn0]

So I've tested it on SS 3.2.1 and the same problem is present there too.

[dhensby]

Ping

[tractorcow]

I said I'd do this just before I was off sick for a week... now I don't have time. :D

[leclerc]

Having 'none' => 'none' in $allowed_children array gave me an error Invalid argument supplied for foreach() in /cms/code/model/SiteTree.php on line 2692

[micschk]

@leclerc: Problem seems to be ClassInfo::subclassesFor() may return NULL instead of an empty array.

A quick fix would be to replace a non-array value with an empty array just before looping over it (SiteTree.php line 2692)

if(!is_array($subclasses)) $subclasses = array();

The allowedchildren method then becomes

public function allowedChildren() {
        $allowedChildren = array();
        $candidates = $this->stat('allowed_children');
        if($candidates && $candidates != "none" && $candidates != "SiteTree_root") {
            foreach($candidates as $candidate) {
                // If a classname is prefixed by "*", such as "*Page", then only that class is allowed - no subclasses.
                // Otherwise, the class and all its subclasses are allowed.
                if(substr($candidate,0,1) == '*') {
                    $allowedChildren[] = substr($candidate,1);
                } else {
                    $subclasses = ClassInfo::subclassesFor($candidate);
                    if(!is_array($subclasses)) $subclasses = array();
                    foreach($subclasses as $subclass) {
                        if($subclass != "SiteTree_root") $allowedChildren[] = $subclass;
                    }
                }
            }
        }

        return $allowedChildren;
    }

[micschk]

Looking into this further, some of the ClassInfo methods are expected to return an array instead of null;
https://github.com/silverstripe/silverstripe-framework/commit/1f0602d42fd9e1c0a4268f3a51aa7f483100a935#commitcomment-17538115

This should probably be fixed at ClassInfo instead of SiteTree...
I've opened a bug & PR...

[dhensby]

thanks @micschk for looking into this.

This is a really sticky point because now we've moved to semantic versioning we can't not return null because it's technically an API change and that can only happen in a new major release (ie: 4).

We can fix the internals of allowedChildren though and that can be a bug fix.

returning null is semi useful in the ClassInfo methods because someone can do if ClassInfo::subClassesOf('Class') == null) //then class is non-existant whereas returning array() is ambiguous (are there no subclasses or is Class missing?

Though we do have a function ClassInfo::exists() for checking this explicitly!

I'm a bit reckless so I probably would accept changing null to array() in ClassInfo but I don't think many will agree.

Let's PR a patch against 3.2 for allowedChildren and then fix the typing in 4

[micschk]

Wouldn't this rather be a fix to have the framework adhere to the API instead of an API change?
I've created a PR, we'll see what happens :) https://github.com/silverstripe/silverstripe-framework/pull/5563

[dhensby]

@micschk it's ambiguous because we have poorly defined public APIs...

We can either be cautious and protect devs that could rely on null being returned (even though it's the docs that say it should be an array) or we are less cautious and we say that this is a bug.

To me, the returning of the null type was completely deliberate and the lack of updates to the PHPdoc was an oversight, this behaviour was not introduced by mistake.

Had we been adhering to semver at the time of that PR, it would not have been allowed and would have to have been an array returned.

[leclerc]

@micschk
Thnx for the code, I'll update my fix cos yours is much better.

[dhensby]

fixed by https://github.com/silverstripe/silverstripe-cms/pull/1711