sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

[RFC] Deprecate `AdminInterface::toString()` and their default implementation #5640

Closed phansys closed 2 years ago

phansys commented 5 years ago

Feature Request

Deprecate AdminInterface::toString() and their default implementation.

https://github.com/sonata-project/SonataAdminBundle/blob/bc279a80e701c200251d802c7910ff782c2770e5/src/Admin/AdminInterface.php#L287-L292

https://github.com/sonata-project/SonataAdminBundle/blob/bc279a80e701c200251d802c7910ff782c2770e5/src/Admin/AbstractAdmin.php#L2444-L2455

This method should be replaced by defining __toString() in order to get the string representation for a given admin (maybe based on AdminInterface::getSubject()).

Regarding the string representation for any object, which is the current responsibility of this method, the replacement should be provided by another service (I don't know which one).

phansys commented 5 years ago

I think we should expand this PR to include all the admin "helper" methods which expect $object as argument instead of using the own $subject property (createObjectSecurity(), getNormalizedIdentifier(), getUrlsafeIdentifier(), id(), etc).

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

VincentLanglet commented 2 years ago

I saw the #5641 PR and the toString method is not always used with the admin subject.

I think it's over-complicated to

And prefer the current implementation.

jordisala1991 commented 2 years ago

I do agree on deprecating the toString but not with the proposed solution. IMO the toString for an object could be a separate responsability outside the admin, with a default implementation that uses the __toString or a default placeholder.

VincentLanglet commented 2 years ago

I do agree on deprecating the toString but not with the proposed solution. IMO the toString for an object could be a separate responsability outside the admin, with a default implementation that uses the __toString or a default placeholder.

Does this would mean a new service, interface and dependency for the admin ? Like the SecurityHandler, the ModelManager, etc...

Or do you mean that this won't be admin-dependant anymore ? It could have just been a dependency to add in the CRUDController, some service, and a twig extension ; but the admin is still using it in

    public function getObjectMetadata(object $object): MetadataInterface
    {
        return new Metadata($this->toString($object));
    }
jordisala1991 commented 2 years ago

Yes, make it non admin dependant, but yes, it requires to deprecate/move the other usages on admin

VincentLanglet commented 2 years ago

Yes, make it non admin dependant, but yes, it requires to deprecate/move the other usages on admin

So this would require people to decorate our service in order to use their own implementation ?

VincentLanglet commented 2 years ago

Closing due to lack of interest