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

Allow ChildAdmin for Unidirectional References #7893

Merged
merged 7 commits into from
Jul 30, 2022

Conversation

VincentLanglet
Copy link
Member

Subject

Closes #7857.

Changing the typehint in the AbstractAdmin is BC.
But for the interface we'll have to wait next major (or should we consider the BC-break now @jordisala1991 ?)

Changelog

### Fixed
- Allow ChildAdmin for Unidirectional References

@VincentLanglet VincentLanglet requested review from a team and jordisala1991 July 28, 2022 21:25
@VincentLanglet
Copy link
Member Author

Static analysis tool are telling me that I'm calling

AdminInterface->setParent($child, null)

So either we ignore the error, either we consider the BC break in the interface.

Also, best would be to add a functional test

@VincentLanglet VincentLanglet marked this pull request as draft July 28, 2022 21:28
@jordisala1991
Copy link
Member

IMHO, this patch should include:

  • Tests about that feature (functional would be nice)
  • Docs about this new way to add childs when they are null related in both ways.
  • And it would be nice if we could detect when someone is passing null but he should pass an argument (not sure if that is possible).

@@ -1302,7 +1302,7 @@ final public function getChild(string $code): AdminInterface
return $this->getChildren()[$code];
}

final public function setParent(AdminInterface $parent, string $parentAssociationMapping): void
final public function setParent(AdminInterface $parent, ?string $parentAssociationMapping): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we avoid call parent?

Something like

if $parent !== null $child->setParent()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set parent is doing

final public function setParent(AdminInterface $parent, ?string $parentAssociationMapping): void
    {
        $this->parent = $parent;
        $this->parentAssociationMapping[$parent->getCode()] = $parentAssociationMapping;
    }

So, we still need to

  • set the parent
  • fill the parentAssociationMapping because later we acces it with
final public function getParentAssociationMapping(): ?string
    {
        if (!$this->isChild()) {
            throw new \LogicException(sprintf(
                'Admin "%s" has no parent.',
                static::class
            ));
        }

        $parent = $this->getParent()->getCode();

        return $this->parentAssociationMapping[$parent];
    }

which already accepts to return null.

@VincentLanglet
Copy link
Member Author

  • Tests about that feature (functional would be nice)

Done

  • Docs about this new way to add childs when they are null related in both ways.

I don't think a lot of doc are needed, I added one line

  • And it would be nice if we could detect when someone is passing null but he should pass an argument (not sure if that is possible).

Not possible IMHO. And technically you can decide to not pass a field even if it exists, for some reason...

The only issue I have now is the psalm/phpstan errors and if we should do a BC-break or not for this "fix".
WDYT ? Should we ignore the errors or update the interface ? @jordisala1991

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
tests/App/config/services.yml Outdated Show resolved Hide resolved
@VincentLanglet VincentLanglet marked this pull request as ready for review July 30, 2022 15:44
@VincentLanglet
Copy link
Member Author

VincentLanglet commented Jul 30, 2022

I ignored the static analysis errors to avoid the bc break. But I'm pretty sure nobody are implementing the interface without extending the abstractAdmin ^^

@VincentLanglet VincentLanglet merged commit b51b9a2 into sonata-project:4.x Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addChild Admin should be available for Unidirectional References (as it was before fieldname was mandatory)
2 participants