silverleague / silverstripe-ideannotator

Generate docblocks for DataObjects, Page, PageControllers and (Data)Extensions
BSD 3-Clause "New" or "Revised" License
46 stars 25 forks source link

hasMany through causes notices #122

Closed lekoala closed 1 year ago

lekoala commented 5 years ago

The current mock data uses this

Array
(
    [SubTeams] => SilverLeague\IDEAnnotator\Tests\SubTeam
    [Comments] => SilverLeague\IDEAnnotator\Tests\TeamComment
    [Supporters] => Array
        (
            [through] => SilverLeague\IDEAnnotator\Tests\TeamSupporter
            [from] => Team
            [to] => Supporter
        )

)

However, it's not supported in the framework (that runs a preg_replace when calling hasMany()) causing array to string conversion notices. I don't see anywhere in the docs that it's possible to use that notation for has_many, only for many_many.

Therefore, should I report a bug to the framework (hasMany should supported nested array => some mapping needs to be done before running preg_replace) or is this module using invalid features?

lekoala commented 5 years ago

For reference, here is the framework code

    public function hasMany($classOnly = true)
    {
        $hasMany = (array)$this->config()->get('has_many');
        if ($hasMany && $classOnly) {
            return preg_replace('/(.+)?\..+/', '$1', $hasMany);
        } else {
            return $hasMany ? $hasMany : array();
        }
    }

and here is the change to fix the issue

    public function hasMany($classOnly = true)
    {
        $hasMany = (array)$this->config()->get('has_many');
        if ($hasMany && $classOnly) {
            $hasMany = array_map(function($a) {
                if(is_array($a)) {
                    return $a['through'];
                }
                return $a;
            }, $hasMany);
            return preg_replace('/(.+)?\..+/', '$1', $hasMany);
        } else {
            return $hasMany ? $hasMany : array();
        }
    }
robbieaverill commented 5 years ago

Yes, "has many through" isn't a a part of the framework API. We should remove support for it from the module/tests.

For background reference, many many through exists as a way of versioning many many relationships. Has many relationships are implicitly versioned, so there's no need for a concept of has many through.

Firesphere commented 1 year ago

I just spend half an hour trying to go through the code and finding the block mentioned above by @lekoala

... :facepalm: ... https://github.com/silverleague/silverstripe-ideannotator/commit/ebedb2495d193a7c2700bf71be96875324103f11